Skip to content

Harden test harness against hangs and waiter leaks#435

Merged
baron merged 16 commits into
repoprompt:mainfrom
morluto:test-hang-risk-hardening
Jul 9, 2026
Merged

Harden test harness against hangs and waiter leaks#435
baron merged 16 commits into
repoprompt:mainfrom
morluto:test-hang-risk-hardening

Conversation

@morluto

@morluto morluto commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR hardens the test harness so missed cancellation, release, socket-read, or subprocess-drain paths fail with bounded assertions instead of hanging the suite.

What was wrong before

Several test helpers could park indefinitely or rely on weak cancellation behavior:

  • checked-continuation gates did not always have bounded release/cancellation cleanup
  • some timeout paths returned nil / false silently, making timeouts easy to under-assert
  • cancellation tests used long real sleeps as “should never finish” placeholders
  • MCP socket reads lacked overall deadlines
  • subprocess helpers could hang while waiting for process exit or draining inherited output pipes

These failures are expensive because they usually show up as stuck test runs rather than direct XCTest failures.

What changed

  • Added shared hang-hardened test fences for bounded release and cancellation waits
  • Split cancellable waits from “ignore cancellation until explicit release” waits for tests that intentionally verify retained resources
  • Hardened codemap and binding-engine test gates so timeout/cancellation paths clean up waiters
  • Made MCP shared-server lease/socket paths safer around queued waiter cancellation and response deadlines
  • Replaced long placeholder sleeps with deterministic cancellation gates
  • Hardened TestProcessRunner / TestGitCommandRunner with process-group cleanup, runner timeouts, and output-drain timeouts

Review order

  1. Tests/RepoPromptTests/Helpers/TestHangHardenedFences.swift
  2. Tests/RepoPromptTests/Helpers/TestProcessRunner.swift
  3. MCP lease/socket test changes
  4. Codemap and binding-engine gate changes
  5. Long-sleep replacement call sites

Out of scope

This PR intentionally does not include CI sharding, broad assertion cleanup, oversized test-file consolidation, or ledger/scoreboard work.

Validation

  • make dev-format
  • make dev-lint
  • make dev-test
  • focused regressions:
    • CodemapBindingEngineInvalidationTests
    • CodemapBindingEngineRootLeaseTests
    • FileSystemContentLoadingConcurrencyTests
    • WindowCloseCoordinatorLifecycleTests
  • contribution preflights passed for commit and push

@baron

baron commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

My hero

morluto and others added 2 commits July 9, 2026 06:04
# Conflicts:
#	Tests/RepoPromptTests/MCP/Control/MCPSharedServerTestLease.swift
#	Tests/RepoPromptTests/MCP/Control/PersistentAgentModeMCPReadFileConnectionTests.swift
#	Tests/RepoPromptTests/WorkspaceContext/Search/SearchPathFilteringTests.swift

@baron baron left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for merge after local review, Fable second-opinion review, follow-up hardening fixes, focused local validation, and green exact-head CI.

@morluto

morluto commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the CI failure:

The failing AgentToolTrackingControllerTests case exposed a pre-existing lifecycle bug rather than a regression introduced by this PR.

The issue was that tool-event observers are stored by (runID, token), but AgentToolTracker.stop() was using the run-wide cleanup path:

unregisterToolObservers(for: runID)

That is correct for true end-of-run cleanup, but too broad for controller-owned teardown. If another observer was registered for the same runID while cleanup was racing with an in-flight delivery barrier, the controller stop path could remove observers it did not own.

The fix changes owner-scoped teardown to be token-scoped:

  • registerToolEventObserver(...) still returns a token
  • AgentToolTracker now stores that token
  • AgentToolTracker.stop() / stop(ifTracking:) unregister only that owned token
  • run-wide unregisterToolObservers(for:) remains available for true run/session cleanup
  • token unregister still waits for that observer’s delivery barrier, preserving the cleanup guarantee without deleting unrelated same-run observers

So the test hardening/sharding did not create this lifecycle issue; it made the race visible by exercising tighter async cleanup timing.

@baron baron left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — agreed this is the better fix. Token-scoped teardown addresses the root cause: controller-owned cleanup was run-wide scoped, so it could remove observers it didn’t register.

I walked the concurrency with Fable: observer delivery leases are acquired synchronously before the fire path’s first suspension, so token removal plus waitUntilIdle() preserves the drain guarantee. The fallback join on an in-flight run-wide unregister also looks right.

I don’t see a blocker in this approach. Current CI failures are now in different suites, so I’d treat those as the remaining blockers to investigate/rerun rather than continuing to debate token-scoped vs test-only cleanup.

@morluto

morluto commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Debugging the CI now.

@baron

baron commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

@morluto I'll stay out of your way and let you handle this just tag me here or there when you want me to look at it. Will just merge it if solid with no pushback for pushback's sake. Thank you!

@morluto

morluto commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

CI is clean, LGTM @baron

@baron baron left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving current head. The token-scoped observer teardown approach looks sound, and CI is green on this revision.

@baron baron merged commit 1b05d2e into repoprompt:main Jul 9, 2026
7 checks passed
@baron

baron commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

@morluto tysm for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants