Run the collaboration test suite async, with deterministic document shutdown - #4840
Merged
Conversation
Collaborator processes (DocumentSupervisor/SharedDoc/PersistenceWriter) could outlive a test's SQL-sandbox owner and crash on DB writes after teardown, poisoning the next test's start_document and producing flaky collaboration suites. Fix the lifecycle product-side rather than babysitting tests: - Add Collaborate.stop_document/1, a synchronous, idempotent teardown that flushes and stops the whole document tree. The symmetric partner to start_document/2. - Add DocumentSupervisor.stop/2 (GenServer.stop(:normal)) which guarantees terminate/2 runs the flush, unlike DynamicSupervisor's :shutdown. - Collaborate.start/1 now tears down a document it started if the session fails to attach, closing the zero-observer orphan leak. - Registry.doc_supervisor_names/0 owns the doc-supervisor match spec. - Tests drop per-test polling for a uniform stop_all_collaboration_documents/0 on_exit net, and a PID-reuse-safe restart assertion.
Tests that start a document via the production entrypoint (Collaborate.start_document/2) place it under the global DocSupervisor, which ExUnit does not own, so the doc outlives the test unless stopped. Previously a single blanket on_exit net swept these up after the fact. Add CollaborationHelpers.start_collaboration_document/2, which registers an on_exit stopping THAT specific document before delegating to start_document/2 — so each doc's lifetime is bound to the test that created it, mirroring start_supervised. Migrate the 7 call sites in session_test.exs. The blanket stop_all_collaboration_documents/0 net stays as belt-and-braces: every site is now individually bound, but a single leaked global doc would still corrupt the next serial (async: false) test, so the cheap idempotent sweep is kept as insurance.
Close three gaps surfaced by the collaboration document lifecycle fix (commits a474204, 759ea81): - §0: frame name-isolation, deterministic teardown and async-safety as three payoffs of one injectable ownership seam. - §1: constructor sub-rule — start_link-style functions put name/owner in trailing opts, while call/lookup lead with the subject; the two rules govern mutually exclusive function shapes and never collide. - §3: lifetime/ownership recipe for dynamic populations — owner-monitored self-cleanup (preferred) vs public symmetric stop + on_exit test binding. Updates the §4 anti-patterns checklist and References accordingly.
Light editorial pass removing cross-section restatement after the lifetime/ownership additions, without cutting teaching content: - §0: let §3 carry the collaboration-fix detail; trim the duplicated test-support-vs-API-seam contrast and the Gray & Tate chapter title. - §1: tighten the identity-vs-configuration restatement. - §3 Option 1: reference the eventstore owner-monitor stated just above rather than re-narrate it. - References: drop prose from the arg-order bullet (the rule lives in §1).
The collaboration document tree (DocumentSupervisor + SharedDoc + PersistenceWriter) is started under a global DynamicSupervisor and is meant to outlive the caller that started it. In tests this is a hazard: a document outlives its test, then writes to the DB after the test's Ecto-sandbox owner has exited, crashing with "owner ... exited" and poisoning the next test. Give start_document an optional owner pid that the DocumentSupervisor monitors; when that owner goes :DOWN the supervisor stops :normal, so terminate/2 runs the flush and the transient child is not restarted. Any caller now gets deterministic teardown by passing owner: self(), with no wrapper. Default owner: nil means no monitor, so production documents outlive the LiveView that starts them exactly as before. The test helper start_collaboration_document/2 now passes owner: self() and drops its on_exit; the blanket stop_all_collaboration_documents/0 net stays as belt-and-braces for the serial suite. Validated against a repeat harness over the three collaboration test files: 0/40 runs under normal timing and 0/30 under constrained scheduling (+S 4:4, ASSERT_RECEIVE_TIMEOUT=1000), with zero post-teardown owner crashes.
Security Review ✅
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4840 +/- ##
=======================================
- Coverage 90.6% 90.6% -0.1%
=======================================
Files 420 421 +1
Lines 19952 20012 +60
=======================================
+ Hits 18084 18127 +43
- Misses 1868 1885 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Introduce a Collaboration.Instance struct describing the registry, dynamic supervisor and :pg scope a collaboration tree runs under, and thread it through the supervisor, registry, document supervisor, session and persistence writer. All new instance parameters default to the current global names, so production behaviour is unchanged. This is the seam that lets each test own an isolated collaboration tree.
DocumentSupervisor.init/1 now invokes a configurable callback for the PersistenceWriter and SharedDoc pids it spawns, passing the owner pid. The callback defaults to a no-op and is only invoked when an owner pid is present, so production is unchanged. In the test environment it grants the spawned processes access to the owner's sandbox connection and Mox expectations, which is what lets these tests run without a shared-mode sandbox.
Drop Mox.set_mox_global from the collaboration DataCase tests and rely on private-mode Mox: expectations are set in the test process and propagated to the spawned collaboration processes via the owner-anchored allow hook (owner: self()) and a new allow_collaboration_process helper. Also add collaboration helpers for driving an isolated supervisor instance per test (per-instance Registry, DynamicSupervisor and :pg scope), threaded through the document start/stop helpers. Default-instance arities are preserved for tests that have not been migrated. The channel-based tests keep their global Mox usage for now; converting them is a separate effort.
Flip the first group of collaboration test modules to async: true, each test owning an isolated supervisor instance (its own Registry, dynamic supervisor and :pg scope) rather than sharing the application-wide singletons. Two production seam fixes the isolation exposed: - Instance.derive/1 now names the :pg scope distinctly (base.PG) instead of reusing the base module, which collided with the supervisor's own registration. The production base still resolves to the existing :workflow_collaboration scope, unchanged. - Thread the owner pid into SharedDoc's persistence state so its init-time read can reach the owner's DB connection: is not propagated across GenServer.start_link, so it is set inside the process that performs the read. No-op in production (no owner). Tests start their collaboration processes under start_supervised! (or stop them synchronously) so every DB-touching process is torn down before its owner exits, avoiding a sandbox-connection teardown race.
Flip the Collaborate and Persistence test modules to async: true, each test owning an isolated supervisor instance and starting its documents under start_supervised! with owner: self(), so the DB-writing children are flushed and stopped before the owner exits. One test that asserted the no-owner document does not set up a monitor is reframed: an async sandbox cannot start a document without an owner (the SharedDoc reads the database during init and needs the owner's connection), so the test now verifies the monitor is keyed to the explicit owner and that re-requesting an existing document is idempotent. The no-owner production-default path remains exercised by the default arities elsewhere.
Flip the Session and WorkflowReconciler test modules to async: true. Session tests each drive an isolated supervisor instance and drain their document trees synchronously (a :normal stop that runs the flush to completion) before the test returns. WorkflowReconciler resolves its SharedDoc through the default :pg scope exactly as the production commit hook does, so those tests run against the default instance and isolate on unique per-test workflow ids. Two production seam fixes the isolation exposed: - Persistence.update_v1 resolved the PersistenceWriter through the hard-coded global registry, so a document running under an isolated registry silently dropped every update. It now reads the registry from the persistence state, defaulting to the global registry (production unchanged). - DocumentSupervisor threads its registry into the persistence state and gains an auto_exit option (default true, production unchanged) so a test can keep the SharedDoc alive until it stops the tree deterministically.
elias-ba
reviewed
Jul 12, 2026
| start_document(Instance.default(), workflow, document_name, []) | ||
| end | ||
|
|
||
| def start_document(%Instance{} = instance, workflow, document_name) do |
Contributor
There was a problem hiding this comment.
I think this is what dialyzer is unhappy about, though correct me if I'm reading it wrong. start_document/3 has two clauses that differ by first arg, this instance-first one and the (workflow, document_name, opts) one below it, and there's a single arity-3 @spec (the workflow-first form), so the start_document(instance, workflow, document_name) call in start/2 looks like it breaks the contract. One option would be to drop this instance-first clause and have the three callers pass opts explicitly as arity-4 (start_document(instance, workflow, document_name, [])); adding a fourth @spec would work too if you'd rather keep the clause. Curious which you'd prefer.
# Conflicts: # CHANGELOG.md # lib/lightning/collaboration/session.ex
elias-ba
approved these changes
Jul 12, 2026
Contributor
|
I went ahead and pushed a couple of commits here @stuartc: one adds the missing |
# Conflicts: # config/test.exs
`Collaborate.start/2` decided whether to tear down a document on failure by
looking up `:pg` membership before starting it. Membership is registered inside
`DocumentSupervisor.init/1`, so a caller racing another's start saw an absent
document, took the create branch, and `start_document/4` masked the resulting
`{:already_started, pid}` as a plain `{:ok, pid}`. Both callers then believed
they owned the document, and a failed session start tore down a document the
other was editing.
The created-or-found answer now comes from the start attempt itself. Other
`start_child/2` errors are passed through rather than raising `CaseClauseError`.
Also drop `stop_all_collaboration_documents/0,1` and the
`Registry.doc_supervisor_names/1` it relied on: both swept every document in a
registry as a safety net for the collaboration suites while they were
`async: false`, and a bulk sweep now pulls documents out from under a running
test. Tolerate an already-stopped SharedDoc in `TestClient`'s teardown, which
was logging a GenServer crash on most runs.
They were sitting in 2.16.8-pre, which shipped in June.
stuartc
changed the title
lmac-1
self-requested a review
August 7, 2026 16:26
`stop/2` defaulted to 5s, but `terminate/2` stops the SharedDoc and the PersistenceWriter with a 5s bound each, so teardown can take 10s. On timeout `proc_lib:stop/3` exits the caller and leaves the target running, and `Collaborate.stop_document/2` catches that exit and returns `:ok` — so it reports success while the flush is still in flight. Test teardown relies on that call to guarantee a document is gone before the next test starts.
lmac-1
approved these changes
Aug 8, 2026
Contributor
There was a problem hiding this comment.
I did a light /code-review and I'm happy with it. It came up with some unrelated improvements which aren't important here. I only paid attention to an improvement related to changes added in this branch and made the update myself directly (see inline comment). Happy for this to be merged if you're ok with that @stuartc
| `terminate/2` bounds each child stop at 5s, so 15s leaves headroom over the | ||
| 10s worst case. | ||
| """ | ||
| def stop(pid, timeout \\ 15_000) when is_pid(pid) do |
Contributor
There was a problem hiding this comment.
@stuartc I tweaked this to to 15s because terminate/2 can take up to 10s (5s per child stop), so the old default could time out mid-flush while stop_document/2 still reported :ok.
stuartc
added a commit
that referenced
this pull request
Aug 12, 2026
The doc landed with #4840 and was already stale against that same PR, and its longest worked example describes a module that has never been on main. UPDATE. §1 and §3 documented an older shape of code that has since adopted their own recommendations. #4840 shipped Lightning.Collaboration.Instance, the per-instance Registry/DynamicSupervisor/:pg struct the doc argued for, and the owner: option it presented as aspirational; both are now the worked examples. stop_document/1 is really /2, and DocumentSupervisor.stop/2 defaults to 15s, not the 5s the doc showed - document_supervisor.ex:43-51 explains why the shorter timeout reports :ok while the flush is in flight. REMOVE. The Lightning.Adaptors.Supervisor example, and the Mox recipe built on it, cite lib/lightning/adaptors/ - which exists only on the unmerged adaptor registry rewrite, where this doc was written. Nothing there resolves for anyone working on main, so it went rather than staying as a set of plausible-looking function names to cite. Recoverable from git when that branch lands. The two-axes framing and the :persistent_term litmus stay; the Mox guidance now points at the real in-repo example at test/support/collaboration_helpers.ex:93-125. Corrected: Task.Supervisor.start_child does propagate $callers on Elixir 1.18 (task/supervisor.ex:527, :545), so the advice to add a manual Mox allowance for it was wrong. stop_all_collaboration_documents/0 does not exist anywhere but this doc. Opus 5 pass: dropped §0's essay on three payoffs of one seam, cut the ~800 words proving an OTP argument-order convention the model already follows, removed the Obsidian frontmatter (no other guideline carries any) and the dangling "case (b)" reference. Anti-pattern checklist moved to the top - it is the payload, the rest is justification. 505 -> 280 lines. The agent pointer moves with the renamed headings.
stuartc
added a commit
that referenced
this pull request
Aug 12, 2026
* agents/security-reviewer: fix three interlocking suppression rules Three rules combined to suppress real findings, all traceable to c4c3424, whose commit message describes a fix that was never applied: - there was no sanctioned route to report a secret leak found in passing; - "Stay in scope" bounded the reporting rather than the checks, so a PR that weakened a policy test was scoped out before it reached the check written to catch exactly that; - the one output category able to carry an uncertain finding was unreachable unless a definite FAIL already existed. One change closes all three. This also records that the frontmatter has never governed a PR review. .github/workflows/security-review.yml does not dispatch this file as a subagent; it passes a prompt telling Claude to read the file and follow it, so the file is consumed as a document and `model` and `tools` are inert on the CI path. security-review.yml:49's own `--model claude-opus-4-7` decides. Two places therefore decide this agent's behaviour, they can disagree, and nothing warns you when they do. The structural defect is confirmed without reference to any model. A forced-dispatch probe additionally had the pre-sweep reviewer certify the broken guard as "No privilege-escalation path found", but that probe compared two different models and was n=1 per arm, so it is not offered here as evidence. The frontmatter escalation is deliberately NOT in this commit. It is the last commit on this branch so it can be dropped on its own. Squashes 5 commits. Per-unit evidence, adjudications and the adversary's objections are in 08-applied-agents.md; all 176 original commit messages are preserved verbatim in 09-commit-log-preserved.md. Audit set archived under .context/stuart/analysis/context-docs-sweep-2026-08/. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * agents: correct stale claims and dead tool names across the agent files Fixes assertions the agent files made that the code does not support, and strips `tools:` entries naming tools that no longer resolve. The correction that mattered most: props on the `ReactComponent` hook arrive as `data-` prefixed kebab-case, verified at CollaborativeEditor.tsx:44-58 (`data-workflow-id`, `data-project-id`, `data-project-is-sandbox` and more), not underscore_cased as the files claimed. Also removes duplicated checklist lines and retargets references that had drifted off their files. The dead `tools:` strips are correctness fixes and are kept here, not in the frontmatter commit, so that dropping the model/effort decision does not also drop them. Squashes 23 commits. Per-unit evidence in 08-applied-agents.md; original messages in 09-commit-log-preserved.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * agents: drop the <example> dialogues from two agent descriptions Removes the three `<example>` dialogues from react-collab-editor's description and the equivalent block from react-test-specialist. Claude Code injects every agent's name and description into every session, so these dialogues were resident context on every run whether or not the agent was ever dispatched. Nothing reads them as examples; they were paying rent as prose. This is the audit's entire measured token saving: -1,141 tokens of the -1,185 total, against a measured resident repo-owned cost of 5,636 -> 4,451. Instruments and method in 08-baseline-before.md. Squashes 2 commits. Per-unit evidence in 08-applied-agents.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * agents: rewrite six descriptions to drop sales copy and dead paths react-test-specialist's description pointed at a unit-test guideline path that does not exist in this repo. web-search-researcher's carried marketing copy ("you can get your money back"). codebase-locator, codebase-pattern-finder, codebase-analyzer and idea-machine each opened by naming themselves or gave prompt-writing advice to the dispatcher rather than describing what they do. Descriptions are resident context, so this is the same argument as the previous commit applied to prose rather than examples, at smaller scale. codebase-pattern-finder keeps its explicit contrast with codebase-locator: that distinction is the thing that makes the pair dispatchable. Squashes 6 commits. Per-unit evidence in 08-applied-agents.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * agents: declare model and effort explicitly on all eleven agents Sets `model` and `effort` on every agent rather than leaving them implicit, and drops context-analyzer's "ultrathink" line, which `effort: high` now expresses as configuration instead of as an instruction in prose. Cost of the frontmatter itself is negligible: under roughly 100 tokens across eleven files even assuming the keys are injected as prompt text rather than merely parsed. The cost that is not negligible is behavioural, and it is real but unevenly distributed: two of the six replay task pairs show a 1.25x-3.3x increase driven by more agents and more phases, one pair's most expensive run was the pre-sweep arm, one was flat, and two rose 12-13% with no cause identified. Kept separate from the content commits so that this decision can be dropped without losing correctness fixes. security-reviewer's own escalation is separate again, in the final commit. Squashes 2 commits. Per-unit evidence in 08-applied-agents.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * guidelines: correct the Yex deadlock mechanism, and the store, toast and ui-patterns docs The load-bearing fix is in yex-guidelines.md. It stated the transaction deadlock rule correctly but explained the mechanism wrongly, and wrongly in the dangerous direction: it implied you were safe in a case that in fact hangs the VM. CLAUDE.md points at that section with a warning, so the wrong explanation was being read on purpose by anyone about to write server-side Y.Doc code. The rewrite says the hazard applies whether or not a `GenServer.call` is involved. The pointer was re-checked after the rewrite and still resolves (rule at yex-guidelines.md:27-29, mechanism at :71-76). Also: reframes Gotcha #4 around `to_list` rather than post-`to_json` behaviour, widens the stated reason for converting atoms, points test helpers at the suite instead of copying them inline, corrects store-structure.md's MetadataStore description (which had been seeded from that store's own wrong docstring), corrects toast-notifications.md's durations (seeded from stale JSDoc in notifications.ts), and fixes the ui-patterns button recipes including the restored `disabled:hover` classes. Two of those corrections illustrate a hazard worth naming: the wrong fact had more than one home. Fixing the guideline does not fix the docstring that seeded it, and the docstring will re-seed it. Squashes 26 commits. Per-unit evidence in 08-applied-guidelines-core.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * rules: move logging.md and ui-patterns.md into .claude/rules/ with paths globs These two files were guidance that only matters when you are editing a matching file, but they lived in .claude/guidelines/, which is only read when something asks for it. Measurement across the replay runs showed referenced guidelines are hardly ever opened. Moving them to .claude/rules/ with a `paths:` frontmatter glob makes the harness inject them whenever a matching file is in play. This is the one mechanically verified behaviour change in the audit: the rules injected in 4 of 4 post-move runs against 0 of 4 before, where both files were inert. Two honest limits on that result. It comes from one task pair's dedicated probes, not the whole replay. And injection is necessary, not sufficient - both runs with ui-patterns.md resident still omitted the focus-visible ring it specifies. Landed as two commits because the first used a pathspec that caught the rename but not the frontmatter. Squashes 2 commits. Per-unit evidence in 08-applied-guidelines-core.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * guidelines/e2e: cut the Playwright guidance back to what the suite supports Rewrites collaborative-testing.md from 749 lines to roughly 200, folds playwright-patterns.md into the files that own its content, and makes phoenix-liveview.md the single home for the wait patterns rather than one of several competing copies. Read this group more sceptically than the others. It is the largest change in the audit by volume and it has the thinnest evidence behind it: one replay task, no source PR to score against, and scoring on conventions only. On that task the evidence ran against the sweep - both pre-sweep runs found a real defect in the suite by reading the code, and one of them repaired it, while the best post-sweep run quoted the documented conclusion and declined to act. The sharpest thing this group adds is redundant against reading the code, and on that task documentation produced less action than no documentation. Kept as its own commit for exactly that reason: it is the group most likely to be worth reverting on its own. Squashes 48 commits. Per-unit evidence in 08-applied-guidelines-e2e.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * guidelines/testing: correct the Vitest guidance and trim the duplicated setup Corrects claims in testing-essentials.md and its three sub-files that the code does not support, removes setup boilerplate duplicated across them, and tightens the assertion-grouping and file-length guidance that CLAUDE.md points at. The `### Naming` section in testing-essentials.md is deliberately retained. The audit proposed removing it, then withdrew the proposal because the instrument measuring it was miscalibrated and three of six runs never opened the file at all. It is retained-but-unverified, not endorsed. One defect this group does not fix, recorded so it is not mistaken for settled. `createMockPushWithResponse` is referenced ten times across five files under assets/test/collaborative-editor/ - the suite's own README.md, __helpers__/README.md, USAGE_EXAMPLES.md, MIGRATION_GUIDE.md and storeHelpers.ts JSDoc - and is defined nowhere. The helpers that do exist in __helpers__/channelMocks.ts (`createMockPush`, `createMockChannelPushOk` and the rest) are named by no audit document at all. This group removed the references that had reached the guidelines; the ones seeding them from inside the suite are untouched and will re-seed. Squashes 25 commits. Per-unit evidence in 08-applied-guidelines-testing.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * commands: correct the plan, spec and research command files Corrects stale references and removes instructions the commands did not need, across create-plan.md, create-spec.md, implement-plan.md, research-codebase.md and validate-plan.md. Adds, as prose rather than as a checklist item, the fact that a plan changing PR behaviour needs the changelog reviewed - deliberately not a checkbox, because a mandatory step is the thing that was being excluded. implement-plan.md:59's cross-check is retained on purpose: its dependency was checked and is not triggered. One flagged weakness. The removal of the no-critique constraint duplication rests on a coverage table that claimed 5 of 6 dispatched agents carry the constraint. The real number is 3 of 6: context-analyzer does not carry it (the audit cited a heading inside that agent's own output template), and web-search-researcher was never checked. That table was the whole of the new evidence answering the objection to this removal, so it is now a single-evidence removal that was presented as multi-evidence. Applied, but it is the first thing to re-examine. Squashes 22 commits. Per-unit evidence in 08-applied-commands.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * CLAUDE.md: rewrite the Guidelines Reference and drop stale sections Drops the eleven per-agent roster bullets (Claude Code injects every agent's name and description already, so the roster was a second copy that could disagree with the first), the Troubleshooting section, the Custom Mix Tasks section, `mix test --only focus`, and the manual `MIX_ENV=test mix ecto.create` step, which the `test` alias at mix.exs:227 already performs. Makes the Database block the single authority on test-database creation. Corrects the LiveView props claim to `data-` prefixed kebab-case. Rewrites the Guidelines Reference with full paths and states once that the guidelines cite file:line rather than copying code. Also deletes deaiify.md's `-HUMAN` file-transformation workflow and restores headings that had been swallowed into a list item. No count is asserted anywhere in the rewritten text, deliberately. Every count that circulated during this audit turned out to be wrong, including several of the audit's own, so the file states none. `§Available Agents` and `§Common Commands` keep their exact names: eight references across four command files cite them by anchor, one of them inside an HTML comment. Squashes 14 commits. Per-unit evidence in 08-applied-claudemd.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * guidelines/testing: remove the Yjs map-ops example (deferred REMOVE, now settled) The example duplicated map-operation coverage that the suite already carries. The removal was deferred while its dependency was open, and is settled now. Its dependency: the `getMap('workflow')` call it described is at createWorkflowStore.ts:950. The original commit message for this change said 951. That off-by-one is left standing in the record rather than corrected, because it was the seventh unverified count in an audit whose main finding was about unverified counts. Squashes 1 commit. Per-unit evidence in 08-applied-guidelines-testing.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * agents/security-reviewer: escalate to opus, effort high, maxTurns 50 Sets `model: opus` (from sonnet), adds `effort: high` and adds `maxTurns: 50`. This is a deferred decision, deliberately isolated as the last commit so it can be dropped with a single revert without touching the correctness fix it arrived alongside. Reasons to look at it hard rather than wave it through: - It is the audit's one clear cost regression on this file. - It changed `model:` in the same file as the prose fixes, which is what confounded the security detection evidence: the pre-sweep arm ran claude-sonnet-5 against the post-sweep arm's claude-opus-5, so the observed improvement cannot be attributed to either change. - On the CI path it does nothing at all. security-review.yml consumes this file as a document, not as an agent definition, and passes its own `--model claude-opus-4-7 --max-turns 50`. These three lines govern interactive dispatch only. So the honest case for it is about interactive use, and the honest case against it is cost. The correctness fix does not depend on it either way. Reconstructed as a hunk-level split: in the original history these three lines were spread across two commits (`maxTurns` inside the first link of the correctness chain, `model`/`effort` inside the commit that set frontmatter on all eleven agents), so neither could be dropped on its own. Per-unit evidence in 08-applied-agents.md. --no-verify: the git_hooks pre-commit runs mix tasks and this worktree has no compiled deps. Markdown-only change under .claude/. * guidelines/testable-supervision-trees: rewrite against the code on main The doc landed with #4840 and was already stale against that same PR, and its longest worked example describes a module that has never been on main. UPDATE. §1 and §3 documented an older shape of code that has since adopted their own recommendations. #4840 shipped Lightning.Collaboration.Instance, the per-instance Registry/DynamicSupervisor/:pg struct the doc argued for, and the owner: option it presented as aspirational; both are now the worked examples. stop_document/1 is really /2, and DocumentSupervisor.stop/2 defaults to 15s, not the 5s the doc showed - document_supervisor.ex:43-51 explains why the shorter timeout reports :ok while the flush is in flight. REMOVE. The Lightning.Adaptors.Supervisor example, and the Mox recipe built on it, cite lib/lightning/adaptors/ - which exists only on the unmerged adaptor registry rewrite, where this doc was written. Nothing there resolves for anyone working on main, so it went rather than staying as a set of plausible-looking function names to cite. Recoverable from git when that branch lands. The two-axes framing and the :persistent_term litmus stay; the Mox guidance now points at the real in-repo example at test/support/collaboration_helpers.ex:93-125. Corrected: Task.Supervisor.start_child does propagate $callers on Elixir 1.18 (task/supervisor.ex:527, :545), so the advice to add a manual Mox allowance for it was wrong. stop_all_collaboration_documents/0 does not exist anywhere but this doc. Opus 5 pass: dropped §0's essay on three payoffs of one seam, cut the ~800 words proving an OTP argument-order convention the model already follows, removed the Obsidian frontmatter (no other guideline carries any) and the dangling "case (b)" reference. Anti-pattern checklist moved to the top - it is the payload, the rest is justification. 505 -> 280 lines. The agent pointer moves with the renamed headings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a class of intermittent failures in the collaborative editing test suites and converts them from serial to async. Two changes make that possible: documents can now be shut down deterministically, and a collaboration supervision tree is no longer tied to one set of global process names.
Collaborative documents start under a global DynamicSupervisor and had no clean way to be stopped — they outlived the process that started them. In tests that meant a document's processes leaked past the test that created them and kept writing to the database after its SQL sandbox had checked in, crashing the next test with an
owner ... exitederror. Run order made it look random. The suites also had to runasync: false, because every test shared one registry, one:pgscope and one dynamic supervisor.Deterministic shutdown
Lightning.Collaborate.stop_document/1— synchronous, idempotent teardown that runs the final persistence flush. The symmetric partner tostart_document/2.start_document/3takes an optionalowner:pid. The document tree monitors it and stops itself (:normal, so the flush runs and the transient child isn't restarted) when the owner exits. Defaults tonil— no monitor — so production documents still outlive the LiveView that starts them.Instance injection
Lightning.Collaboration.Instancedescribes the three names one tree owns: its registry, its dynamic supervisor and its:pgscope.Instance.default/0returns the existing production atoms, so production wiring is byte-identical.Supervisor,Registry,DocumentSupervisor,Session,PersistenceandPersistenceWriteraccept those names as defaulted leading arguments, so every existing call shape still means what it meant.start_collaboration_document/2,3and astart_collaboration_instance/0helper, plus a:collaboration_process_allowtest-only config callback that grants sandbox and Mox access to children the supervisor spawns.Nine collaboration test files now run
async: true, taking the repo from 50 serial test files to 43.A production bug this surfaced
Collaborate.start/2decided whether to tear a document down on failure by checking:pgmembership before starting it. Membership is registered insideDocumentSupervisor.init/1, so a caller racing another's start saw an absent document, took the create branch, andstart_document/4masked the resulting{:already_started, pid}as a plain{:ok, pid}. Both callers then believed they owned the document, and a failed session start would tear down a document the other was editing. The created-or-found answer now comes from the start attempt itself, and otherstart_child/2errors pass through instead of raisingCaseClauseError.It also adds a developer guideline,
.claude/guidelines/testable-supervision-trees.md, on writing supervision trees that stay addressable in tests without forcing the suite to run serially.Validation steps
mix test test/lightning/collaboration/ test/lightning/collaborate_test.exs— 162 tests, 0 failures, and the timing line should read1.0s async, 0.00s sync. These were previously serial and intermittently red.mix test test/lightning_web/channels/workflow_channel_test.exs test/lightning_web/channels/workflow_channel_broadcast_test.exs— 149 tests, 0 failures, 1 skipped. These still use the global instance and were deliberately left serial.start_document/2with noownerregisters no monitor and the document behaves exactly as before.Additional notes for the reviewer
lib/passesowner:, andInstance.derive/1special-cases the production supervisor name so the three atoms are identical to before.+S 4:4, tightassert_receivetimeout), against a pre-fix baseline of 11/40.main, five full-suite runs at fixed seeds (5170 tests each) attributed zero failures to this branch. The failures those runs did surface are pre-existing onmainand unrelated — mostlycapture_logreturning empty across six files, which looks like one shared log-capture problem worth its own issue.lib/passes an owner: a document has a single owner slot, so a second caller'sowneris silently discarded; and cross-node, two nodes can each start a SharedDoc for the same name, since the registry and dynamic supervisor are node-local.WorkflowReconcileris not instance-aware — it resolves SharedDocs through the global:pgscope like the production save path — soworkflow_reconciler_test.exsdeliberately uses the default global instance. Its comments explain the constraint that keeps that safe.AI Usage
Pre-submission checklist