-
Notifications
You must be signed in to change notification settings - Fork 168
fix(sweep): gate maintenance-sweep workflow ingest by mtime fingerprint #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -733,6 +733,11 @@ if (require.main === module) { | |||||||||||||||||
| const { broadcast } = require("./websocket"); | ||||||||||||||||||
| const { importCompactions } = require("../scripts/import-history"); | ||||||||||||||||||
| const { transcriptCache } = require("./routes/hooks"); | ||||||||||||||||||
| // Per-session newest workflow-artifact mtime already ingested by this sweep, | ||||||||||||||||||
| // so step 3 below skips sessions whose workflow files are unchanged (the same | ||||||||||||||||||
| // cheap fingerprint startWorkflowPoll uses). Declared once so it persists | ||||||||||||||||||
| // across sweep ticks. | ||||||||||||||||||
| const sweepWorkflowSeen = new Map(); | ||||||||||||||||||
| setInterval(() => { | ||||||||||||||||||
| // 1. Stale session cleanup — batch agent updates to avoid N+1 queries | ||||||||||||||||||
| const stale = cleanupDb.stmts.findStaleSessions.all("__periodic__", STALE_MINUTES); | ||||||||||||||||||
|
|
@@ -809,9 +814,29 @@ if (require.main === module) { | |||||||||||||||||
| // 3. Scan active sessions for Workflow-tool run journals (issue #167). | ||||||||||||||||||
| // Catches workflows that complete without a subsequent hook and flips | ||||||||||||||||||
| // launch-detected "running" rows to "completed" once their journal lands. | ||||||||||||||||||
| const { ingestWorkflowsForSession } = require("./lib/workflow-ingest"); | ||||||||||||||||||
| const { ingestWorkflowsForSession, workflowsMaxMtime } = require("./lib/workflow-ingest"); | ||||||||||||||||||
| // Forget fingerprints for sessions that are no longer active so the map | ||||||||||||||||||
| // can't grow without bound over the process lifetime. | ||||||||||||||||||
| const activeIds = new Set(active.map((r) => r.session_id)); | ||||||||||||||||||
| for (const id of sweepWorkflowSeen.keys()) { | ||||||||||||||||||
| if (!activeIds.has(id)) sweepWorkflowSeen.delete(id); | ||||||||||||||||||
| } | ||||||||||||||||||
| for (const row of active) { | ||||||||||||||||||
| if (!row.tp) continue; | ||||||||||||||||||
| // Skip sessions whose workflow artifacts are unchanged since the last | ||||||||||||||||||
| // ingest — the same cheap mtime fingerprint startWorkflowPoll uses. | ||||||||||||||||||
| // Without this the sweep full-re-parses every workflow journal and every | ||||||||||||||||||
| // inner agent-*.jsonl for every active session every cycle; on a large | ||||||||||||||||||
| // corpus that re-parse exceeds the sweep interval, sweeps overlap, and | ||||||||||||||||||
| // the event loop pegs (dashboard stops responding — white page). | ||||||||||||||||||
| let mtime = 0; | ||||||||||||||||||
| try { | ||||||||||||||||||
| mtime = workflowsMaxMtime(row.tp); | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| mtime = 0; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (mtime === 0 || sweepWorkflowSeen.get(row.session_id) === mtime) continue; | ||||||||||||||||||
| sweepWorkflowSeen.set(row.session_id, mtime); | ||||||||||||||||||
| ingestWorkflowsForSession(cleanupDb, { id: row.session_id, transcript_path: row.tp }) | ||||||||||||||||||
| .then((changed) => { | ||||||||||||||||||
|
Comment on lines
840
to
841
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Transient Ingestion Failures Cause Permanent SkipSetting On subsequent sweep ticks, the sweep will skip this session because To make this robust against transient failures, we should delete the session from
Suggested change
|
||||||||||||||||||
| if (!changed || changed.length === 0) return; | ||||||||||||||||||
|
|
@@ -820,6 +845,9 @@ if (require.main === module) { | |||||||||||||||||
| if (sess) broadcast("session_updated", sess); | ||||||||||||||||||
| }) | ||||||||||||||||||
| .catch((err) => { | ||||||||||||||||||
| // Forget the fingerprint so the next sweep retries this session | ||||||||||||||||||
| // instead of skipping it until its artifacts change again. | ||||||||||||||||||
| sweepWorkflowSeen.delete(row.session_id); | ||||||||||||||||||
| console.warn( | ||||||||||||||||||
| `[SWEEP] Workflow scan failed for session ${row.session_id}:`, | ||||||||||||||||||
| err?.message || err | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unbounded Memory Growth in
sweepWorkflowSeenThe
sweepWorkflowSeenmap is declared in the outer scope and persists across all sweep ticks. However, there is currently no mechanism to evict keys when sessions become inactive, completed, or abandoned. Over time, this will lead to unbounded memory growth (a memory leak) as new sessions are created and eventually archived.To prevent this, we should evict inactive sessions from the map at the start of each sweep cycle.