Repository navigation
fix(crews): drop never-created seats from the roster before the launch report - #377
Merged
Merged
Conversation
…h report A seat reserved before a restart mid-launch kept its roster row with no thread, so the report listed it on the roster and its name stayed taken. The launch reporter now drops every row the watched proposal minted whose thread does not exist, inside the Crew's serialize section and before the report gate, for initial rosters and additions, on the live path and the boot sweep. Seats the launch reported created or not started keep their rows, and a failed drop leaves the report owed for the next sweep. Closes #376 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The addition test now leaves the roster seat without a thread too, so it fails if the report stops filtering by the proposal's reserved seats. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
bryantderosier
requested review from
BastiHu,
Jacksondr5 and
tyler-barton-horizon
September 29, 2026 15:54
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
BastiHu
approved these changes
Sep 29, 2026
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
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.
Problem
If the server stops after a Crew launch has recorded its seats but before every seat thread exists, a seat with no thread stays on the Crew's roster. The live launch drops a never-created seat, but only while it's running. After a restart, the startup sweep reports the seat as "its thread was never created" but leaves the row. That row then blocks the Captain:
request_crew_memberrefuses the seat name as a duplicate,addMembersreports a conflict, and the row counts toward the 12-seat cap. The launch report also lists the seat on its roster while saying it isn't there. This applies to additions as well as initial rosters. Found while closing #346 (#376).What I changed
apps/server/src/j5/a2a/CrewLaunchReporter.ts→dropSeatsNeverCreated, called at the start ofwatch, before the reporter's gate. It returns early unless the proposal is approved, linked to a Crew, and not yet reported. Then, in onecrews.serialize(crewInstanceId)section, it re-reads the Crew, takes only the rows this proposal minted (crewSeatReservedBy), skips any seat the launch reported as created or not started, and removes the ones whose thread is a genuine not-found (getThreadProjectionIfPresentreturns null). The rest ofwatchis unchanged apart from reindenting, sogit diff -wshows the real change. It already re-reads the roster afterwards, so the report no longer lists the dropped seat and names it as not created.CrewLaunchReporterShape.watchdoc: states the drop, that a failed drop fails the report, and the locking assumption.apps/server/src/j5/a2a/CrewLaunchService.ts→ comment inspawnAndBriefonly: the report now drops a row the launch couldn't.apps/server/src/j5/a2a/CrewLaunchReporter.test.ts: four tests, plus awrapCrewsfixture option and anapprovedAdditionhelper.Why this shape
The fix goes where the missing seat is already measured. The launch report is the one place that runs on both the live path and the startup sweep, so dropping there fixes both without a new sweep or recovery job. That keeps to "repair beats edge-case machinery" (#327) and to the ruling that Crews have no general restart recovery (#346, closed).
A missing thread can safely count as final under the Crew's lock. A launch approves its proposal inside the step that holds
crews.serializefor the Crew. So once the reporter holds that lock, the step has either finished or died with the process, and nothing recreates a seat for a proposal that's no longer open.A failed drop fails the whole report instead of being logged and skipped. Nothing gets posted or stamped, so the next startup sweep tries again, rather than a durable report claiming a seat is gone while its row is still there.
I rejected a lock-ordering race test because it couldn't fail reliably on the unfixed code without adding a test seam to production code.
Invariants
crews.serializesection.watch's existingcatchCause.watchis never called while holdingcrews.serializefor the same Crew (the executor isn't reentrant). Today it's called only fromCrewProposalService.resolveafterfulfilreturns, and from the startup sweep.crews.serializeis taken and released before the reporter's gate, so the two locks never nest.crews.serializefor the Crew. That's true today; nothing enforces it, and thewatchdoc says so.Surfaces
packages/contracts)crews.md,agent-tools.md("left off the roster") and FORK.md already promise this. The fix makes them true across a restart.Out of scope
crew_versionon removal, and identity checks insideremoveMembers.Upgrade and data
No migration. Existing reported proposals keep any leftover row (see Out of scope). Older clients and servers are unaffected.
Verification
vp test run apps/server/src/j5/a2a/CrewLaunchReporter.test.ts apps/server/src/j5/a2a/CrewLaunchService.test.ts apps/server/src/j5/a2a/CrewProposalService.test.ts apps/server/src/j5/a2a/CrewSeatFinishNotifier.test.ts: 4 files, 56 tests passed.cd apps/server && npx tsc --noEmit: exit 0.vp linton the three changed files: clean.origin/j5/main'sCrewLaunchReporter.tsand the new tests:- second:.second.extra. The test also drops the roster seat's thread, to prove rows another proposal minted are untouched. It fails if thecrewSeatReservedByfilter is removed.Review focus
crews.serializesection indropSeatsNeverCreated, and that it's released beforegate.withPermit.watchwhile holding the same Crew's lock.Closes #376
Claude Opus 5.5 via J5 Code (Claude Code), with Codex GPT-6 Sol and GPT-6-Astra and Claude Fable 5.1 reviewing
🤖 Generated with Claude Code