ZacharyZhang-NY
e74dafcd36
Three independently ugly bugs that all fed the same log flood:
* Electron's `WebFrameMain.send` writes every "Render frame was disposed"
to stderr via `console.error`. When the worker fans a second
agent-event past a reloading renderer, we produced thousands of
identical lines and stalled the main loop on stderr I/O. Added a
`mainFrame.detached` guard so the send is short-circuited before
Electron's own catch block fires, and installed a targeted filter in
`suppress-warnings.ts` (now actually imported from the entry point)
so the specific `console.error("Error sending from webFrameMain:"…)`
noise never reaches the terminal. Any other payload still surfaces.
* `dispatchAgentMessage` accepted the human-reader sentinel as a wake
target when an agent replied to `__board__` — `wakeAgentIfPossible`
then called `db.getAgent("__board__")`, threw, and the dispatcher
kept repeating on every send. Strip `HUMAN_SENDER_ID` at
`resolveMessageWakeTargets`, and keep a defense-in-depth guard in the
heartbeat so a stray caller (future automation fan-out) cannot
reintroduce the crash.
* Pi's skills loader rejects any SKILL.md whose frontmatter `name`
diverges from its parent directory. The four bundled `my-writing-style`
copies declared `name: writing-style-skill`, so every install emitted
a warning and dropped the skill. They also lived as orphan gitlinks
(mode 160000 without .gitmodules), and the global `*.md` gitignore
hid every other SKILL.md from version control. Fixed the frontmatter
to match the dir, converted the dangling submodules to plain trees,
and allowlisted `builtin-skills/**/*.md` so a fresh clone carries the
catalogue instead of silently losing it.
116 lines
3.4 KiB
Markdown
116 lines
3.4 KiB
Markdown
# Condition-Based Waiting
|
|
|
|
## Overview
|
|
|
|
Flaky tests often guess at timing with arbitrary delays. This creates race conditions where tests pass on fast machines but fail under load or in CI.
|
|
|
|
**Core principle:** Wait for the actual condition you care about, not a guess about how long it takes.
|
|
|
|
## When to Use
|
|
|
|
```dot
|
|
digraph when_to_use {
|
|
"Test uses setTimeout/sleep?" [shape=diamond];
|
|
"Testing timing behavior?" [shape=diamond];
|
|
"Document WHY timeout needed" [shape=box];
|
|
"Use condition-based waiting" [shape=box];
|
|
|
|
"Test uses setTimeout/sleep?" -> "Testing timing behavior?" [label="yes"];
|
|
"Testing timing behavior?" -> "Document WHY timeout needed" [label="yes"];
|
|
"Testing timing behavior?" -> "Use condition-based waiting" [label="no"];
|
|
}
|
|
```
|
|
|
|
**Use when:**
|
|
- Tests have arbitrary delays (`setTimeout`, `sleep`, `time.sleep()`)
|
|
- Tests are flaky (pass sometimes, fail under load)
|
|
- Tests timeout when run in parallel
|
|
- Waiting for async operations to complete
|
|
|
|
**Don't use when:**
|
|
- Testing actual timing behavior (debounce, throttle intervals)
|
|
- Always document WHY if using arbitrary timeout
|
|
|
|
## Core Pattern
|
|
|
|
```typescript
|
|
// ❌ BEFORE: Guessing at timing
|
|
await new Promise(r => setTimeout(r, 50));
|
|
const result = getResult();
|
|
expect(result).toBeDefined();
|
|
|
|
// ✅ AFTER: Waiting for condition
|
|
await waitFor(() => getResult() !== undefined);
|
|
const result = getResult();
|
|
expect(result).toBeDefined();
|
|
```
|
|
|
|
## Quick Patterns
|
|
|
|
| Scenario | Pattern |
|
|
|----------|---------|
|
|
| Wait for event | `waitFor(() => events.find(e => e.type === 'DONE'))` |
|
|
| Wait for state | `waitFor(() => machine.state === 'ready')` |
|
|
| Wait for count | `waitFor(() => items.length >= 5)` |
|
|
| Wait for file | `waitFor(() => fs.existsSync(path))` |
|
|
| Complex condition | `waitFor(() => obj.ready && obj.value > 10)` |
|
|
|
|
## Implementation
|
|
|
|
Generic polling function:
|
|
```typescript
|
|
async function waitFor<T>(
|
|
condition: () => T | undefined | null | false,
|
|
description: string,
|
|
timeoutMs = 5000
|
|
): Promise<T> {
|
|
const startTime = Date.now();
|
|
|
|
while (true) {
|
|
const result = condition();
|
|
if (result) return result;
|
|
|
|
if (Date.now() - startTime > timeoutMs) {
|
|
throw new Error(`Timeout waiting for ${description} after ${timeoutMs}ms`);
|
|
}
|
|
|
|
await new Promise(r => setTimeout(r, 10)); // Poll every 10ms
|
|
}
|
|
}
|
|
```
|
|
|
|
See `condition-based-waiting-example.ts` in this directory for complete implementation with domain-specific helpers (`waitForEvent`, `waitForEventCount`, `waitForEventMatch`) from actual debugging session.
|
|
|
|
## Common Mistakes
|
|
|
|
**❌ Polling too fast:** `setTimeout(check, 1)` - wastes CPU
|
|
**✅ Fix:** Poll every 10ms
|
|
|
|
**❌ No timeout:** Loop forever if condition never met
|
|
**✅ Fix:** Always include timeout with clear error
|
|
|
|
**❌ Stale data:** Cache state before loop
|
|
**✅ Fix:** Call getter inside loop for fresh data
|
|
|
|
## When Arbitrary Timeout IS Correct
|
|
|
|
```typescript
|
|
// Tool ticks every 100ms - need 2 ticks to verify partial output
|
|
await waitForEvent(manager, 'TOOL_STARTED'); // First: wait for condition
|
|
await new Promise(r => setTimeout(r, 200)); // Then: wait for timed behavior
|
|
// 200ms = 2 ticks at 100ms intervals - documented and justified
|
|
```
|
|
|
|
**Requirements:**
|
|
1. First wait for triggering condition
|
|
2. Based on known timing (not guessing)
|
|
3. Comment explaining WHY
|
|
|
|
## Real-World Impact
|
|
|
|
From debugging session (2025-10-03):
|
|
- Fixed 15 flaky tests across 3 files
|
|
- Pass rate: 60% → 100%
|
|
- Execution time: 40% faster
|
|
- No more race conditions
|