Skip to content

Commit b07a628

Browse files
authored
fix(session): watchdog respects per-session TTL and suppress test noise (#64) (#64)
Watchdog used global TIMEOUT (5 min) to kill all sessions, ignoring per-session timeout query param. Pydoll creates 1-hour persistent sessions that got killed every ~6 minutes, causing scrape failures. - Store timeout query param in session.ttl (was hardcoded to 0) - Watchdog uses per-session TTL when set, falls back to global default - Upgrade KILLING log from info→warn for production visibility - Capture pydoll subprocess stderr (only surface on failure) - Suppress debug logger in unit tests via env override
1 parent b4cf1c5 commit b07a628

6 files changed

Lines changed: 149 additions & 14 deletions

File tree

docs/SESSION_LEAK_POSTMORTEM.md

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,51 @@ If you see memory climbing again:
115115
```
116116
Safe — active sessions reference dirs by handle, not path.
117117

118+
## Part 2: Watchdog vs Per-Session Timeout (2026-03-06)
119+
120+
### Summary
121+
122+
After deploying the `destroySession` fix (Part 1), the watchdog correctly cleaned up sessions. But sessions were STILL going stale — the watchdog fired every 60s, killing ~2 sessions aged 367-420s. The `destroySession` fix treated the symptom (orphaned data dirs); this fix addresses the root cause (why sessions go stale in the first place).
123+
124+
### Root Cause
125+
126+
The watchdog used global `TIMEOUT` env var (300s = 5 min) instead of per-session `ttl`.
127+
128+
Pydoll's AhrefsSessionManager creates persistent Chrome sessions with `timeout=3600000` (1 hour) via the WebSocket query param. The limiter (queue library) correctly used this as the job timeout. But the watchdog ignored it entirely and used `TIMEOUT + 60s` = 360s as the kill threshold.
129+
130+
**The math:**
131+
- Watchdog threshold: `TIMEOUT + 60s` = 360s
132+
- Watchdog poll interval: 60s
133+
- Expected stale age: 360-420s (threshold + poll variance)
134+
- Observed stale ages: 367-420s — exact match
135+
136+
### The Cascade
137+
138+
```
139+
t=0: Pydoll connects with timeout=3600000 (1 hour)
140+
t=0-5m: Session alive, scrapes running normally
141+
t=6m: Watchdog kills session (360s threshold + poll variance)
142+
t=6m: Pydoll's WebSocket closes → "browser_session_closed"
143+
t=6m: Any in-flight scrape fails → "Turnstile timeout" / "workflow failed"
144+
t=6m+: Pydoll recreates session (AhrefsSessionManager._ensure_session)
145+
t=12m: Watchdog kills again...
146+
```
147+
148+
### Fix
149+
150+
1. **Store per-session timeout**: `session.ttl = timeout` query param (was hardcoded to `0`)
151+
2. **Watchdog respects TTL**: `maxAge = s.ttl > 0 ? s.ttl + 60_000 : defaultMaxAgeMs`
152+
3. **Upgraded log levels**: `KILLING browser session``warn` (was `info`, invisible in prod)
153+
154+
Sessions with no explicit timeout (`ttl=0`) still use the global `TIMEOUT + 60s` default (unchanged behavior). Persistent sessions (e.g., Ahrefs `ttl=3600000`) now have watchdog threshold of 3,660s (1 hour + 60s buffer).
155+
156+
### Corrected Causal Chain
157+
158+
The Part 1 postmortem incorrectly attributed scrape failures to "10+ GB working set starving Chrome for memory." The VM has 50 GB RAM and the container has no memory limit. The real cause of scrape failures was the watchdog killing persistent sessions every ~6 minutes:
159+
160+
1. Orphaned data dirs → memory growth (real, but not the cause of scrape failures)
161+
2. Watchdog killing persistent sessions → scrape failures (the actual root cause)
162+
118163
## Effect v4 Lesson Learned
119164

120165
`Effect.promise` treats rejections as DEFECTS (unrecoverable). `Effect.ignore` only catches typed ERRORS.

src/browsers/browser-launcher.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,9 @@ export class BrowserLauncher {
144144
return this.handlePageConnection(req);
145145
}
146146

147-
// Parse launch options
147+
// Parse launch options and per-session timeout
148148
const launchOptions = this.parseLaunchOptions(req, router);
149+
const timeout = req.parsed.searchParams.get('timeout');
149150

150151
// Determine user data directory
151152
const manualUserDataDir = this.getManualUserDataDir(launchOptions);
@@ -196,7 +197,7 @@ export class BrowserLauncher {
196197
routePath: router.path,
197198
startedOn: Date.now(),
198199
trackingId,
199-
ttl: 0,
200+
ttl: timeout ? +timeout : 0,
200201
userDataDir,
201202
};
202203

src/session/cf/cf-sites.integration.test.ts

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -432,17 +432,27 @@ describe.concurrent('CF Solver Multi-Site', () => {
432432

433433
const PYDOLL_DIR = '/Users/peter/Developer/catchseo/packages/pydoll-scraper';
434434

435-
/** Run a command with proxy env var and return stdout. */
435+
/** Run a command with proxy env var and return stdout. Stderr is captured and only surfaced on failure. */
436436
function runWithProxy(args: string[], timeoutMs: number): string {
437437
const proxy = process.env.LOCAL_MOBILE_PROXY;
438438
if (!proxy) throw new Error('LOCAL_MOBILE_PROXY required for pydoll tests');
439-
return execFileSync('uv', ['run', 'pydoll', ...args], {
440-
cwd: PYDOLL_DIR,
441-
env: { ...process.env, LOCAL_MOBILE_PROXY: proxy },
442-
encoding: 'utf-8',
443-
timeout: timeoutMs,
444-
maxBuffer: 10 * 1024 * 1024,
445-
});
439+
try {
440+
return execFileSync('uv', ['run', 'pydoll', ...args], {
441+
cwd: PYDOLL_DIR,
442+
env: { ...process.env, LOCAL_MOBILE_PROXY: proxy },
443+
encoding: 'utf-8',
444+
timeout: timeoutMs,
445+
stdio: ['ignore', 'pipe', 'pipe'],
446+
maxBuffer: 10 * 1024 * 1024,
447+
});
448+
} catch (err: unknown) {
449+
const execErr = err as { stderr?: string; stdout?: string; message?: string };
450+
const stderr = execErr.stderr || '';
451+
const stdout = execErr.stdout || '';
452+
throw new Error(
453+
`pydoll ${args[0]} failed:\n${stderr}\n\nstdout:\n${stdout}`,
454+
);
455+
}
446456
}
447457

448458
describe('Pydoll Pipeline', () => {

src/session/session-lifecycle-manager.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ export class SessionLifecycleManager {
184184
}
185185

186186
if (!keepOpen) {
187-
this.log.info(`KILLING browser session ${session.id}: numbConnected=${connected} keepUntil=${keepUntil} force=${force}`);
187+
this.log.warn(`KILLING browser session ${session.id}: numbConnected=${connected} keepUntil=${keepUntil} force=${force}`);
188188
await Effect.runPromise(this.destroySession(browser, session));
189189
}
190190

@@ -277,20 +277,24 @@ export class SessionLifecycleManager {
277277
* Uses destroySession — the same cleanup pipeline as normal close.
278278
* Impossible to diverge from the close() path.
279279
*/
280-
startWatchdog(maxSessionAgeMs: number): void {
280+
startWatchdog(defaultMaxAgeMs: number): void {
281281
const lifecycle = this;
282282
this.watchdogFiber = Effect.runFork(
283283
Effect.fn('watchdog.tick')(function*() {
284284
const now = Date.now();
285285
const stale = lifecycle.registry.toArray()
286-
.filter(([, s]) => now - s.startedOn > maxSessionAgeMs);
286+
.filter(([, s]) => {
287+
// Use per-session TTL if set, otherwise fall back to global default
288+
const maxAge = s.ttl > 0 ? s.ttl + 60_000 : defaultMaxAgeMs;
289+
return now - s.startedOn > maxAge;
290+
});
287291

288292
if (stale.length > 0) {
289293
lifecycle.log.warn(`Watchdog: ${stale.length} stale session(s)`);
290294
yield* Effect.all(
291295
stale.map(([browser, session]) => {
292296
lifecycle.log.warn(
293-
`Watchdog: force-closing ${session.id} (age=${Math.round((now - session.startedOn) / 1000)}s)`,
297+
`Watchdog: force-closing ${session.id} (age=${Math.round((now - session.startedOn) / 1000)}s, ttl=${session.ttl}ms, numbConnected=${session.numbConnected})`,
294298
);
295299
return lifecycle.destroySession(browser, session).pipe(
296300
Effect.timeout('20 seconds'),

src/session/session-lifecycle.test.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,80 @@ describe('SessionLifecycleManager', () => {
218218
});
219219
});
220220

221+
describe('watchdog per-session TTL', () => {
222+
let registry: SessionRegistry;
223+
let lifecycle: SessionLifecycleManager;
224+
225+
beforeEach(() => {
226+
registry = new SessionRegistry();
227+
lifecycle = new SessionLifecycleManager(registry);
228+
});
229+
230+
it('watchdog respects per-session TTL over global default', async () => {
231+
const browser = makeBrowser('b1');
232+
// Session with ttl=3600000 (1 hour), 400s old — past global default but within TTL
233+
const session = makeSession('s1', {
234+
ttl: 3_600_000,
235+
startedOn: Date.now() - 400_000,
236+
});
237+
registry.register(browser, session);
238+
239+
// Global default would kill at 360s, but session TTL is 1 hour + 60s buffer
240+
lifecycle.startWatchdog(360_000);
241+
242+
// Wait for one watchdog tick (60s schedule, but first tick fires immediately)
243+
await new Promise((r) => setTimeout(r, 1_500));
244+
245+
// Session should still be alive (ttl=1h, only 400s old)
246+
expect(registry.size()).toBe(1);
247+
248+
// Cleanup
249+
lifecycle.clearTimers();
250+
await lifecycle.shutdown();
251+
});
252+
253+
it('watchdog kills session with no TTL using global default', async () => {
254+
const browser = makeBrowser('b1');
255+
// Session with ttl=0 (no per-session TTL), 400s old — past global 360s default
256+
const session = makeSession('s1', {
257+
ttl: 0,
258+
startedOn: Date.now() - 400_000,
259+
});
260+
registry.register(browser, session);
261+
262+
lifecycle.startWatchdog(360_000);
263+
264+
// Wait for one watchdog tick
265+
await new Promise((r) => setTimeout(r, 1_500));
266+
267+
// Session should be killed (no per-session TTL, 400s > 360s global default)
268+
expect(registry.size()).toBe(0);
269+
270+
lifecycle.clearTimers();
271+
});
272+
273+
it('watchdog kills session that exceeds its own TTL', async () => {
274+
const browser = makeBrowser('b1');
275+
// Session with ttl=300000 (5 min), but 400s old (past ttl + 60s buffer = 360s)
276+
const session = makeSession('s1', {
277+
ttl: 300_000,
278+
startedOn: Date.now() - 400_000,
279+
});
280+
registry.register(browser, session);
281+
282+
// Global default is much larger, but per-session TTL should take priority
283+
lifecycle.startWatchdog(7_200_000);
284+
285+
// Wait for one watchdog tick
286+
await new Promise((r) => setTimeout(r, 1_500));
287+
288+
// Session should be killed (400s > ttl 300s + 60s buffer = 360s)
289+
expect(registry.size()).toBe(0);
290+
291+
lifecycle.clearTimers();
292+
});
293+
});
294+
221295
effectDescribe('acquireSession', () => {
222296
effectIt.effect('registers on acquire, removes on scope close', () =>
223297
Effect.gen(function*() {

vitest.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ export default defineConfig({
1515
include: ['src/**/*.test.ts'],
1616
exclude: ['src/**/*.integration.test.ts'],
1717
globals: false,
18+
env: { DEBUG: '' },
1819
fakeTimers: {
1920
toFake: undefined,
2021
},

0 commit comments

Comments
 (0)