fix SchedulerService isolate-spawn race (T-106)
_startTicker fired Isolate.spawn(...).then((iso) => _isolate = iso) and returned. If _stopTicker landed before the spawn future resolved, _isolate was still null at kill time and the just-spawned isolate (with its Timer.periodic) leaked forever. Track the spawn as _isolateReady and have _stopTicker await it before killing. Same shape as the NativePty fix from T-96. dispose() is now async; the single caller in facade.dart already sat inside an async dispose chain and just needed the await. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.7
@@ -1713,3 +1713,9 @@ INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by,
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-102', 'status', 'backlog', 'in_progress', NULL, '2026-05-17 18:58:56', '2026-05-17 18:58:56', '2026-05-17 18:58:56', NULL, '7fa075d7232df5302ffdfb5d1c3d79f2', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-101', 'status', 'in_progress', 'done', NULL, '2026-05-17 19:01:39', '2026-05-17 19:01:39', '2026-05-17 19:01:39', NULL, '56e8da656c5fc53af241900e6313805e', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-102', 'status', 'in_progress', 'done', NULL, '2026-05-17 19:01:39', '2026-05-17 19:01:39', '2026-05-17 19:01:39', NULL, '7f09d2a1501d3d072e071822c191f253', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-103', 'status', 'backlog', 'in_progress', NULL, '2026-05-17 19:03:18', '2026-05-17 19:03:18', '2026-05-17 19:03:18', NULL, '2635d907f94402ec506d3b56dfc25b21', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-106', 'status', 'backlog', 'in_progress', NULL, '2026-05-17 19:03:18', '2026-05-17 19:03:18', '2026-05-17 19:03:18', NULL, 'cc39b3fd718e99423823e640950ccf1b', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-113', 'status', 'backlog', 'in_progress', NULL, '2026-05-17 19:03:18', '2026-05-17 19:03:18', '2026-05-17 19:03:18', NULL, 'cc8605833ba5cb97e068eb5a6d57d8f4', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-106', 'status', 'in_progress', 'done', NULL, '2026-05-17 19:18:43', '2026-05-17 19:18:43', '2026-05-17 19:18:43', NULL, '8dccc83b93c5005db744a100a5b3baea', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-113', 'status', 'in_progress', 'done', NULL, '2026-05-17 19:18:43', '2026-05-17 19:18:43', '2026-05-17 19:18:43', NULL, '8f7812831e564005eb796708543c54b3', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-103', 'status', 'in_progress', 'done', NULL, '2026-05-17 19:18:43', '2026-05-17 19:18:43', '2026-05-17 19:18:43', NULL, 'c7d1b76cb6854efb7e9990cc7204d899', 1) ON CONFLICT(hash) DO NOTHING;
|
||||
|
||||
@@ -1623,3 +1623,48 @@ INSERT INTO tickets (id, type, parent_id, title, description, status, priority,
|
||||
4. Spot-check other doc references to dissolved components.
|
||||
|
||||
Source: consultants.md "Docs — Findings — [Critical]".', 'done', 'high', NULL, NULL, NULL, '2026-05-17 18:47:29', '2026-05-17 19:01:39', NULL, 'a7d378267119361f2b2188977881a666', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-115', 'story', NULL, 'startup project picker; "always open this project" override', 'Startup flow change: on launch, always show the project picker screen unless the user has explicitly checked an "always open this project" box for a specific project (sticky default).
|
||||
|
||||
**Acceptance:**
|
||||
1. Default startup: project picker is the first screen, regardless of last-opened state.
|
||||
2. Picker has an "always open this project on launch" checkbox per project entry.
|
||||
3. If exactly one project has that flag set, startup skips the picker and opens that project directly.
|
||||
4. The flag is per-project, persisted in app settings (probably under `app.startupProject`).
|
||||
5. The checkbox can be unticked from the welcome view to restore picker-first behavior.
|
||||
|
||||
**Notes:** today the app auto-opens `app.lastProject` on boot (kernel/src/project.dart `openLast`). The new flow inverts the default — picker is the steady state, sticky-open is opt-in per project.
|
||||
|
||||
**Out of scope:** multi-project workspaces, project groups, recent-project ordering changes.
|
||||
|
||||
**Source:** user request 2026-05-17.', 'backlog', 'medium', NULL, NULL, NULL, '2026-05-17 19:10:21', '2026-05-17 19:10:21', NULL, 'ce156fb92fbad8ba9bf860a4eb71d1ac', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-116', 'bug', 'T-97', 'integration_test/theme_picker_test.dart hangs pumpAndSettle on theme.pick', 'When running `flutter test integration_test/theme_picker_test.dart` against the real boot path, calling `services.commands.execute(''theme.pick'')` followed by `await tester.pumpAndSettle()` does not settle — the test hangs for 60+ seconds and never reaches the `Select theme` / `Cancel` assertions. Likely an animation loop or a never-completing Future in the modal mount path.
|
||||
|
||||
Blocks T-103 from adding integration tests to `make push-check` (the other two integration tests pass after viewport + assertion fixes).
|
||||
|
||||
**Acceptance:**
|
||||
1. `flutter test integration_test/theme_picker_test.dart` completes in <10s.
|
||||
2. Both assertions (`Select theme` visible after `theme.pick`; gone after `Cancel`) pass.
|
||||
3. T-103''s `push-check-full` can include integration tests without the theme-picker hanging.
|
||||
|
||||
Source: surfaced 2026-05-17 during T-103 work.', 'backlog', 'medium', NULL, NULL, NULL, '2026-05-17 19:14:31', '2026-05-17 19:14:31', NULL, 'df0c6bdd610410d4bd27bfeb2be9acbc', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-113', 'task', 'T-97', 'quick-wins: governance + docs reconciliation', 'Batch of small consultant findings that share a domain (governance/docs):
|
||||
|
||||
1. **Amend D-66** to match current floor location/value: the floor lives in `pubspec.yaml` `coverage_floor:` (single source of truth as of this session), not `coverage/floor.txt`. Add a dated amendment line.
|
||||
2. **Reconcile `licenses.yaml` vs `pubspec.yaml`** — `test` version drift (`1.25.8` vs `1.30.0`), phantom `lints 5.0.0` not in pubspec. Two-step-commit rule from D-42 is being violated.
|
||||
3. **Triage stale Q-records** — Q-1/Q-2/Q-3 ("defer until Tier 1 is in real use") are due now that Tier 1 has shipped. Q-25 (body text face) is de facto resolved by D-43/D-44.
|
||||
4. **Drop `--no-fatal-infos` from `ci/test.sh`** now that the import queue is clear (post test-sweep this session).
|
||||
5. Add a `.claude/skills/README.md` inventory.
|
||||
|
||||
Source: consultants.md (Docs + Tests + Security Findings — Minor).', 'done', 'low', NULL, NULL, NULL, '2026-05-17 18:48:24', '2026-05-17 19:18:43', NULL, '6988f892ed906cb397bea5068fdcd30e', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-106', 'bug', 'T-97', 'fix SchedulerService._startTicker isolate-spawn race', '`scheduler.dart:71`: `Isolate.spawn(_isolateEntry, _port!.sendPort).then((iso) => _isolate = iso)`. If `_stopTicker()` runs before the spawn future completes, `_isolate` is still null, nothing is killed, and the just-spawned isolate (with its `Timer.periodic`) leaks forever.
|
||||
|
||||
Same race shape we just fixed in `NativePty` via `_readerReady` (T-96).
|
||||
|
||||
**Fix:** track the spawn future as `_isolateReady`; `_stopTicker` awaits it before killing.
|
||||
|
||||
Source: consultants.md "Code quality — Findings — [Major]".', 'done', 'medium', NULL, NULL, NULL, '2026-05-17 18:47:51', '2026-05-17 19:18:43', NULL, 'b529416bab7c66fde471f990671f3b25', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (id, type, parent_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('T-103', 'task', 'T-97', 'add test-integration + smoke-bundle to make push-check', '`make push-check` runs `decisions-validate test-core test test-a11y coverage-gate changelog-gate`. `test-integration` and `smoke-bundle` are omitted — a boot-order regression sails through the pre-push gate.
|
||||
|
||||
**Acceptance:** `make push-check` runs the integration test layer + smoke-bundle. Wall-clock budget acceptable for pre-push (target <2 min total). If they''re too slow, gate them behind a separate `make push-check-full` and document.
|
||||
|
||||
Source: consultants.md "Tests — Findings — [Major] make push-check does not run integration tests".', 'done', 'high', NULL, NULL, NULL, '2026-05-17 18:47:39', '2026-05-17 19:18:43', NULL, 'cbae1915f1c89a8a2b05da71148d77dc', 1) ON CONFLICT(id) DO UPDATE SET type=excluded.type, parent_id=excluded.parent_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
|
||||
@@ -98,6 +98,8 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit.
|
||||
- `README.md` rewritten to match current architecture; `docs/initial-plan.md`
|
||||
bannered as historical; new `docs/architecture.md` describes today's
|
||||
shape (T-101).
|
||||
- `SchedulerService._stopTicker` now awaits the in-flight isolate spawn
|
||||
before killing — closes the same race shape we fixed in PTY (T-106).
|
||||
- Terminal panes now render bold attributes with a real bold weight —
|
||||
bundled JetBrainsMono Bold + BoldItalic are registered with the
|
||||
`JetBrainsMono` family at `weight: 700`. The painter's bold
|
||||
|
||||
@@ -238,7 +238,7 @@ class KernelServices {
|
||||
focus.dispose();
|
||||
project.dispose();
|
||||
extensions.dispose();
|
||||
scheduler.dispose();
|
||||
await scheduler.dispose();
|
||||
await log.dispose();
|
||||
messages.dispose();
|
||||
await events.dispose();
|
||||
|
||||
@@ -38,6 +38,13 @@ class SchedulerService {
|
||||
StreamSubscription<dynamic>? _sub;
|
||||
StreamSubscription<dynamic>? _projectSub;
|
||||
|
||||
/// Tracks the spawn future so [_stopTicker] can await it before
|
||||
/// killing — otherwise a stop racing a still-spawning isolate
|
||||
/// leaves `_isolate` null at kill time and the just-spawned isolate
|
||||
/// (with its `Timer.periodic`) leaks forever. Same race shape we
|
||||
/// fixed in `NativePty` via `_readerReady` (T-96).
|
||||
Future<Isolate?>? _isolateReady;
|
||||
|
||||
/// Listen for project lifecycle events. The periodic ticker only runs
|
||||
/// while a project is open — no wasted cycles on the welcome screen.
|
||||
void start() {
|
||||
@@ -47,8 +54,8 @@ class SchedulerService {
|
||||
|
||||
/// Start the periodic ticker and fire an immediate first cycle so
|
||||
/// all panels refresh without waiting for the first interval.
|
||||
void _startTicker() {
|
||||
_stopTicker();
|
||||
Future<void> _startTicker() async {
|
||||
await _stopTicker();
|
||||
|
||||
// Stagger the initial ticks to avoid a rebuild storm on project open.
|
||||
var delay = 0;
|
||||
@@ -68,14 +75,26 @@ class SchedulerService {
|
||||
_events.emit(SchedulerTick(tier: tier));
|
||||
}
|
||||
});
|
||||
Isolate.spawn(_isolateEntry, _port!.sendPort).then((iso) => _isolate = iso);
|
||||
_isolateReady = Isolate.spawn(_isolateEntry, _port!.sendPort);
|
||||
_isolateReady!.then((iso) => _isolate = iso).catchError((_) => null);
|
||||
}
|
||||
|
||||
void _stopTicker() {
|
||||
Future<void> _stopTicker() async {
|
||||
// Await any in-flight spawn so we never miss killing an isolate
|
||||
// that's mid-creation — see _isolateReady.
|
||||
if (_isolateReady != null) {
|
||||
try {
|
||||
final pending = await _isolateReady;
|
||||
_isolate ??= pending;
|
||||
} catch (_) {
|
||||
// Spawn failed; nothing to kill.
|
||||
}
|
||||
}
|
||||
_sub?.cancel();
|
||||
_port?.close();
|
||||
_isolate?.kill(priority: Isolate.immediate);
|
||||
_isolate = null;
|
||||
_isolateReady = null;
|
||||
_port = null;
|
||||
_sub = null;
|
||||
}
|
||||
@@ -97,8 +116,8 @@ class SchedulerService {
|
||||
}
|
||||
}
|
||||
|
||||
void dispose() {
|
||||
Future<void> dispose() async {
|
||||
_projectSub?.cancel();
|
||||
_stopTicker();
|
||||
await _stopTicker();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -220,5 +220,22 @@ void main() {
|
||||
bus.emit(const ProjectClosed());
|
||||
// Smoke — no throw.
|
||||
});
|
||||
|
||||
test('dispose immediately after ProjectOpened awaits the in-flight spawn (T-106)', () async {
|
||||
// Race window: ProjectOpened triggers _startTicker which calls
|
||||
// `Isolate.spawn(...)`; if dispose lands before the spawn future
|
||||
// resolves, the old code left _isolate=null and the just-spawned
|
||||
// ticker leaked forever. Now dispose awaits _isolateReady before
|
||||
// killing.
|
||||
final bus = DaemonBus();
|
||||
final s = SchedulerService(bus);
|
||||
s.start();
|
||||
bus.emit(const ProjectOpened(path: '/tmp/x'));
|
||||
// Don't wait for the spawn to settle — dispose must do that itself.
|
||||
await s.dispose();
|
||||
// No throw, no leaked isolate (test runner would flag a lingering
|
||||
// isolate by failing to exit cleanly). Reaching this line is the
|
||||
// assertion.
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user