test(office): prove a dropped children-completed wake gets recovered

[nova28]

Tip

PR walkthrough: Open the visual walkthrough

Adds the regression test a github-actions review bot asked for on #3271: proof that ParentWakeReconciler actually recovers a parent's on_children_completed wake when the edge-triggered dispatch fails, since #3271 downgraded that failure's log level from Error to Warn on the strength of an until-now-untested assumption.

Today: If queueChildrenCompletedRun fails partway (e.g. a transient DB error), the parent's wake is silently dropped on the edge path and only logged at Warn — nothing in the test suite proved the reconciler's recovery sweep actually re-queues it.
After this: A new test drives that exact failure through ParentWakeReconciler.Tick and asserts a second dispatch attempt with the same operation id, one queued run, and a receipt whose delivery id matches. A control test alongside it proves a successful edge dispatch is not double-queued by the same sweep.
Who hits this: Contributors changing the office wake/reconciler path — this closes the exact gap the review bot flagged on #3271, which was previously an assumption baked into a comment, not a test.
Scope: Standalone follow-up to #3271; no sibling PRs.
Not here: No production code changes — this is test-only coverage. Two pre-existing, unrelated test failures on this environment are noted below (not caused by this diff, not fixed here).

Validation

$ go test -tags fts5 ./internal/office/service/... -run 'TestParentWakeReconciler|TestWakeOperationID' -count=1
ok  	github.com/kandev/kandev/internal/office/service	0.928s

$ go test -tags fts5 -race ./internal/office/service/... -run 'TestParentWakeReconciler|TestWakeOperationID' -count=5
ok  	github.com/kandev/kandev/internal/office/service	2.455s

$ make fmt && make typecheck && make lint && make lint-format
✓ all clean (0 lint issues across backend, web, harness, specs, architecture)

$ cd apps/web && pnpm run i18n:ratchet
✓ i18n new-code ratchet — no UI source added or modified

make test (full suite) has two pre-existing failure sets on this environment, both proven unrelated to this diff (identical failures reproduce on a clean checkout with none of this branch's changes):

1. internal/worktree + several dependents (internal/task/service, internal/task/handlers, internal/launcher, internal/system/storage/workspaces, internal/agent/..., internal/agentctl/...) — this macOS sandbox's /var/folders symlink layout trips the worktree manager's own path-safety checks. Reproduces identically on a scratch clone of the merge base.
2. internal/office/repository/sqlite's TestMigrate_PriorityIdempotent — fails deterministically on current main (3d042e9d8) with no relation to this branch (git diff origin/main HEAD --stat shows only the one test file this PR adds). Introduced by an unrelated commit between the old and current main tip; filed as a follow-up (kandev task 4f422031-3cd1-4893-85ee-3e9f77ec2f63).

Possible Improvements

Low risk — test-only change, zero production code touched. Review noted a few test-rigor nitpicks (a doc-comment could more precisely name the injected failure point, and the control test could assert on the dispatcher directly rather than only counting runs); judged non-blocking and left as-is to keep the diff minimal.

Checklist

• If I do not have repository write access and this is a large architectural change, I discussed the direction in a linked issue before opening this PR.
• This PR contains one logical change; unrelated work is split into separate PRs.
• I have performed a self-review of my code.
• I have manually tested my changes and they work as expected.
• My changes have tests that cover the new functionality and edge cases.
• If my change touches UI files (apps/web/), I have added or updated Playwright e2e tests in apps/web/e2e/ and verified them with make test-e2e.
• I checked whether this affects public docs in docs/public/** and updated them or noted why no docs change is needed.

[github-actions]

Claude finished @nova28's task in 5m 48s —— View job

---

Findings

Reviewed commit 617104db — apps/backend/internal/office/service/scheduler_wake_reconciler_test.go (+171/-0).

---

Suggestion (recommended, doesn't block)

1. Misleading failErr message names the wrong failure point — scheduler_wake_reconciler_test.go:389

   • Issue: fmt.Errorf("injected get child set key failure") implies the simulated failure is at GetChildSetKey inside queueChildrenCompletedRun, but oneShotFailureDispatcher.HandleTrigger actually returns this error after GetChildSetKey has already succeeded — the injection point is the engine-dispatch call, not the key lookup.
   • Why: A future reader tracing the test without the PR description will expect to find a GetChildSetKey stub, not find one, and have to trace the whole dispatch stack to understand what's really being simulated.
   • Fix: fmt.Errorf("injected on_children_completed dispatch failure") (or similar). See inline comment.

2. oneShotFailureDispatcher.calls is unguarded — inconsistent with fakeDispatcher pattern — scheduler_wake_reconciler_test.go:338

   • Issue: fakeDispatcher (in event_subscribers_engine_test.go) protects calls with sync.Mutex and exposes a copy via Calls(). oneShotFailureDispatcher accesses d.calls directly both on write (line 344) and from the test body (lines 401, 431), with no lock.
   • Why: Safe today because newTestServiceWithBus uses a synchronous bus (SetSyncHandlers(true)), but the asymmetry with the established pattern adds fragility. A future test that uses this type with an async bus, or forgets SetSyncHandlers, will race without any compile-time warning.
   • Fix: Add sync.Mutex + a Calls() []dispatcherCall accessor to match fakeDispatcher. See inline comment.

3. Control test (DoesNotDoubleQueueASuccessfulEdgeDispatch) can't rule out a reconciler dispatch attempt — scheduler_wake_reconciler_test.go:495

   • Issue: The test asserts run count stays at 1 after Tick, but queueRunDispatcher doesn't record calls. If ListStuckParents's receipt-exclusion logic were broken, the reconciler would attempt a second dispatch and the DB unique index would silently absorb it — the run count would still be 1 and the test would still pass.
   • Why: Acknowledged in the PR description as a known gap. Not a correctness problem today, but limits the test's diagnostic value as a regression fence.
   • Fix: Wrap queueRunDispatcher in a thin counting or recording decorator and assert dispatch count. See inline comment for a minimal sketch.

---

Summary

Severity	Count
Blocker	0
Suggestion	3

Verdict: Ready with suggestions.

The two core tests are logically sound:

• TestParentWakeReconciler_RecoversFailedEdgeDispatch correctly drives a failed edge dispatch and verifies the reconciler re-delivers with the same operation ID, a queued run, and a matching receipt — exactly the property the gap analysis on fix(office): stop parent tasks waking twice when children finish #3271 asked for.
• TestParentWakeReconciler_DoesNotDoubleQueueASuccessfulEdgeDispatch provides meaningful non-vacuity proof that the recovery assertions discriminate on the edge path's outcome.

All three findings above are suggestions the PR author already acknowledged; none block correctness or introduce a regression risk.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: 11575fe2-082f-413e-8d7a-290170179fe0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Team

Run ID: 26f42213-7971-450c-9f0d-0874a0b9c7a2

📥 Commits

Reviewing files that changed from the base of the PR and between 3d042e9 and 64badf3.

📒 Files selected for processing (1)

• apps/backend/internal/office/service/scheduler_wake_reconciler_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Summary

Summary by CodeRabbit

• Tests
  • Added coverage confirming failed workflow wake-ups are retried on a subsequent reconciliation.
  • Added coverage confirming successful wake-ups are not queued again during later reconciliation.
  • Verified retried wake-ups preserve the original operation identifier.

Walkthrough

The test suite adds helpers for simulated dispatch failures and child completion events. It verifies retry behavior after failed edge dispatches and prevents duplicate queue entries after successful dispatches.

Changes

Parent wake reconciler validation

Layer / File(s)	Summary
Dispatch failure and event helpers apps/backend/internal/office/service/scheduler_wake_reconciler_test.go	Adds a one-shot failing dispatcher and a helper that publishes task.moved child completion events.
Reconciler retry and deduplication tests apps/backend/internal/office/service/scheduler_wake_reconciler_test.go	Verifies retry with the same operation ID after a failed edge dispatch. Verifies that a successful dispatch does not queue a second run.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 64bad

The change adds regression coverage for retrying failed parent wake dispatches and preventing duplicate runs after successful dispatches, without modifying production behavior. It is ready to merge.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: regression coverage for recovery of a dropped children-completed wake.
Description check	✅ Passed	The description explains the problem, expected behavior, test coverage, validation commands, known unrelated failures, scope, risks, and checklist. Optional sections are appropriately omitted.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.}

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This test-only PR adds regression coverage for recovery of a failed edge-triggered parent wake and a control for successful edge delivery.

• Exercises a one-shot dispatcher failure followed by ParentWakeReconciler.Tick.
• Verifies operation-ID continuity, queued-run creation, and wake-receipt delivery.
• Adds a successful-edge control, although its run-count assertion does not directly detect a redundant reconciler dispatch.

Confidence Score: 4/5

The PR appears safe to merge, with two non-blocking test-rigor issues that should be tightened to make the claimed regression coverage precise.

The recovery test exercises the dispatcher-failure path successfully, but its documentation claims a different failure boundary, while the successful-edge control can miss a redundant dispatch hidden by run idempotency.

Files Needing Attention: apps/backend/internal/office/service/scheduler_wake_reconciler_test.go

Important Files Changed

Filename	Overview
apps/backend/internal/office/service/scheduler_wake_reconciler_test.go	Adds useful wake-recovery regression coverage, but misidentifies the injected failure boundary and does not directly observe duplicate dispatches in the control test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Child reaches Done] --> B[Edge wake dispatch]
  B -->|Injected dispatcher failure| C[No run or receipt]
  C --> D[ParentWakeReconciler Tick]
  D --> E[Dispatch same operation ID]
  E --> F[Queued run and receipt]
  B -->|Success| G[Queued run]
  G --> H[Reconciler sweep should not dispatch again]

Loading

_{Reviews (1): Last reviewed commit: "test(office): cover ParentWakeReconciler..." | Re-trigger Greptile}

[nova28]

CI on 95d2419 (this round's fixup commit) shows 2 failures, both from the same root cause: Backend (windows) → TestPollModeGrace_StopJoinsFinalScan in apps/backend/internal/agentctl/server/process/workspace_poll_mode_grace_test.go:216 (Run Backend Tests is the required-checks gate and just relays that failure).

This is unrelated to this PR's diff (one file, internal/office/service/scheduler_wake_reconciler_test.go, disjoint package/subsystem from internal/agentctl/server/process). Evidence it's CI flake rather than a regression from this round's push:

• Same job (Backend (windows)) passed on the immediately-preceding commit 64badf3f on this same branch/merge-base, before this round's test-only diff landed.
• go test -race -run '^TestPollModeGrace_StopJoinsFinalScan$' -count=50 passes 50/50 locally (macOS).
• The test's own doc comment already documents Windows-specific scheduling overlap in the git-poll/monitor loops it exercises (workspace_poll_mode_grace_test.go:186-190), consistent with runner-timing variance rather than a deterministic bug.

I don't have admin rights on this repo to trigger gh run rerun --failed as a contributor. Could a maintainer re-run the failed Backend (windows) job (run 34174867852) when convenient?

[carlosflorencio]

Added a small follow-up commit (2904323) that clarifies the injected failure boundary and verifies that both edge and reconciler dispatches target the expected parent and trigger. This makes the regression test more precise without changing production behavior. Thanks for the contribution.