Skip to content

fix(sync): bound OpenCode watcher updates - #1189

Closed
berenddeboer wants to merge 9 commits into
kenn-io:mainfrom
berenddeboer:fix/opencode-incremental-watch
Closed

fix(sync): bound OpenCode watcher updates#1189
berenddeboer wants to merge 9 commits into
kenn-io:mainfrom
berenddeboer:fix/opencode-incremental-watch

Conversation

@berenddeboer

@berenddeboer berenddeboer commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Problem

During active OpenCode use, agentsview repeatedly consumed roughly 244% to 937% CPU. Profiling showed 89.5% of CPU time under OpenCode SQLite parsing: each write to opencode.db or its WAL was expanded to every virtual OpenCode session, and every streamed part update reparsed the active session’s full, increasingly large transcript. The work therefore scaled with archive size and transcript history instead of the changed batch.

Fix

OpenCode SQLite watcher events now use the durable event journal to select only changed session IDs. The cursor records a rowid boundary and SQLite container state, advances only after successful processing, and falls back to full discovery for journal regression, replacement, incompatible schemas, or non-session aggregates. Hybrid storage shadowing and persistent archived-session behavior remain intact.

Streaming part updates are coalesced per session until a user message, completed or failed assistant message, session event, or two-second quiet period. Pending sessions live in the cursor, scheduled retries ensure the final quiet update is not stranded, and failed parses retain both the cursor and retry. Content identified by the journal bypasses coarse metadata freshness checks while retaining post-parse content fingerprint filtering.

Validated with make test-short, go vet -tags fts5 ./..., focused parser and sync regressions, Linux and Windows Go tests, PostgreSQL integration tests, E2E tests, and the benchmark gate. An isolated CPU profile replaying 1,900 intermediate part events and 100 completion events showed full session parsing at 1.57% of CPU samples.

@roborev-ci

roborev-ci Bot commented Jul 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (ac91718)

Code review found two medium-severity issues in the journal-backed OpenCode change.

Medium

  • internal/parser/opencode_provider.go:445 — Journal failures can drop watcher-triggered updates. Cursor or delta errors abort changed-path classification instead of using the existing full-container fallback. Compatible forks with different event columns or malformed journal rows may therefore miss updates even though normal session discovery works. Treat journal failures as an unavailable optimization: retain the previous cursor, fall back to SourcesForChangedPath, and validate required journal columns before declaring support.

  • internal/parser/opencode_provider.go:665 — Shadowing checks scale with changed sessions × project directories. Every changed SQLite session scans all storage project directories for a shadowing JSON session, making watcher work unbounded in hybrid archives. Maintain or pass a session-ID-indexed storage lookup, and add a regression that increases project-directory cardinality while measuring lookup work.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 5m50s

@roborev-ci

roborev-ci Bot commented Jul 19, 2026

Copy link
Copy Markdown

roborev: Combined Review (69723b4)

The change needs revision: three medium-severity synchronization correctness and scalability issues remain.

Medium

  • internal/parser/opencode_provider.go:468 — Full-discovery fallback may permanently skip SQLite changes. Fallback SQLite sources are returned without ContentChanged, so same-mtime or WAL-only updates can fail freshness checks. The new cursor may then be promoted without parsing the changes. Mark fallback SQLite sources as ContentChanged before cursor promotion.

  • internal/sync/engine.go:2889 — Resync cursors advance before the rebuilt database is committed. syncAllLocked updates change cursors while writing the temporary database. If metadata copying, index rebuilding, or swapping later fails, the active archive retains old data while its cursors acknowledge changes written only to the discarded database. Stage cursor updates until a successful swap, or restore the previous cursors on every pre-swap failure.

  • internal/parser/opencode_provider.go:665 — Changed-session processing scales with the entire project archive. Each changed SQLite session calls os.ReadDir across all storage project directories, making watcher work scale as changed sessions × total projects and repeatedly allocating large directory listings. Include project_id in the targeted metadata query and inspect its storage path directly, or maintain another bounded session-to-project lookup.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 6m43s

@berenddeboer
berenddeboer marked this pull request as draft July 20, 2026 07:11
@roborev-ci

roborev-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown

roborev: Combined Review (c2f7d54)

Changes improve bounded OpenCode synchronization, but four medium-severity correctness and scaling issues remain.

Medium

  • internal/sync/engine.go:2916syncAllLocked advances source cursors while resync is still writing the temporary archive. If metadata copying, FTS rebuilding, or the database swap later fails, the active archive retains a cursor that skips changes present only in the discarded database. Stage cursor updates until a successful swap, or restore prior cursors on every abort path.

  • internal/sync/engine.go:992 — Retry callbacks call background-context SyncPaths, while Close waits for callbacks that have started. A retry racing with shutdown can perform an uncancellable database scan or block behind another sync indefinitely. Use an engine-owned cancellable context, cancel it before waiting in Close, call SyncPathsContext, and propagate the context through changed-path classification.

  • internal/parser/opencode_provider.go:665storageSessionExists scans every storage project directory for each changed SQLite session, making hybrid-root watcher work O(changed sessions × total projects). The existing cardinality test puts all 5,000 files in one project and misses this scaling dimension. Use a bounded session-ID lookup or maintained storage index, and add a regression test varying project-directory cardinality.

  • internal/parser/opencode.go:171 — A WAL checkpoint changes container state without adding an event row, causing the cursor to be treated as incomplete and triggering full session enumeration. Because main-database checkpoint events are watched, routine checkpointing can repeatedly defeat bounded updates. Rebaseline checkpoint-only transitions without full discovery while preserving fallback for logical unjournaled changes, and add a WAL-checkpoint scaling test.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 6m11s

@roborev-ci

roborev-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown

roborev: Combined Review (60f49d0)

The review found two medium-severity reliability issues in OpenCode journal synchronization.

Medium

  • internal/parser/opencode.go:170 — Database replacement detection only occurs when the journal high-water mark is unchanged. If the SQLite file is replaced while preserving the boundary event and adding newer events, the replacement is incorrectly treated as a complete delta, potentially missing unrelated session changes. Detect inode/device replacement independently of cursor advancement, force full discovery, and add a regression test for this scenario.

  • internal/sync/engine.go:1059 — Persistent parse failures trigger an unbounded one-second retry loop. Because the cursor does not advance, every retry reprocesses the full journal delta, causing indefinite CPU usage, database traffic, and log noise. Use capped exponential backoff or limit retries until a new watcher event or periodic full sync occurs.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m7s

@roborev-ci

roborev-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown

roborev: Combined Review (b8ad2dc)

Medium-severity synchronization issues remain; no security-boundary vulnerabilities were identified.

Medium

  • internal/parser/opencode_provider.go:469 — Full-discovery failures are not retried. The fallback returns cursors without retry descriptors, so rescheduleFailedChangedPathRetries receives an empty slice when parsing fails. The promised one-time retry never occurs, leaving updates stale until another filesystem event or periodic sync. Include a non-pending retry entry for each fallback cursor so success cancels an existing timer and failure schedules the retry.

  • internal/parser/opencode.go:170 — Database replacement detection is unreliable on Windows. Replacement detection relies on inode/device values, which are always zero on Windows. A replacement database retaining the old boundary event but adding rows can incorrectly produce Complete=true, causing only post-cursor events to be processed and unjournaled sessions to be missed. Implement Windows file identity or fail closed to full discovery when replacement cannot be ruled out, and add platform-independent replacement coverage.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m23s

@roborev-ci

roborev-ci Bot commented Jul 20, 2026

Copy link
Copy Markdown

roborev: Combined Review (ae2741f)

Code review found two medium-severity issues in the OpenCode watcher synchronization.

Medium

  • internal/parser/opencode_provider.go:652 — Incremental duplicate detection misses cross-project storage copies

    Incremental detection checks only storage/session/<SQLite project_id>/<session_id>.json, while full discovery resolves duplicate session IDs across every storage project. If the canonical copy is under another directory such as global, a journal event can parse the shadowed SQLite row, overwrite the archived storage transcript, and remain incorrect because of storage trust caching.

    Fix: Maintain a bounded session-ID index across storage projects, or fall back when the absence of a storage duplicate cannot be proven. Add a journal-backed regression test where the storage directory differs from the SQLite project_id.

  • internal/parser/opencode_provider.go:708 — Cancellation does not interrupt SQLite work

    The cancellable watcher calls ListOpenCodeEventDelta and ListOpenCodeSessionMetaByID, but these helpers use context-free SQLite operations and scan the entire changed batch. Shutdown can therefore remain blocked on busy waits or numerous per-session queries.

    Fix: Thread context.Context through these helpers, use BeginTx, QueryContext, and QueryRowContext, and check cancellation while iterating events and session IDs.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 7m10s

@roborev-ci

roborev-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (7a18311)

The review found three medium-severity correctness and scalability issues; no security regressions were identified.

Medium

  • internal/parser/opencode_provider.go:656 — Incremental sync can scan the entire archive

    In hybrid storage/SQLite roots, a changed SQLite session without a same-project storage file forces full storage and database discovery. Routine watcher work therefore scales with total archive size rather than the changed batch.

    Suggested fix: Maintain a session-ID index for storage duplicates or persist source hints so cross-project duplicate resolution stays proportional to the changed session IDs.

  • internal/parser/opencode_provider.go:107 — Missing SQLite database disables incremental processing elsewhere

    Storage-mode resolution supplies a prospective database path even when no SQLite database exists. Cursor capture then fails and discards cursors from all other valid roots/providers, disabling incremental watcher processing in mixed configurations.

    Suggested fix: Capture a cursor only when the resolved database path is an existing regular file, and isolate failures so one unavailable container does not invalidate successful cursors.

  • internal/sync/engine.go:893 — Transient SQLite locks can drop watcher updates

    Changed-path classification errors discard retry descriptors. With the journal’s 100 ms busy timeout, a transient lock can drop the final watcher update, leaving the archive stale until another event or periodic sync.

    Suggested fix: Preserve or create a pending retry for non-cancellation classification errors without advancing the cursor, and add an engine-level locked-database recovery test.


Reviewers: 2 done | Synthesis: codex, 12s | Total: 8m51s

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant