Skip to content

fix(sync): bound OpenCode watcher fallback work - #1306

Closed
rodboev wants to merge 1 commit into
kenn-io:mainfrom
rodboev:pr/1208-reconciliation-scoping
Closed

fix(sync): bound OpenCode watcher fallback work#1306
rodboev wants to merge 1 commit into
kenn-io:mainfrom
rodboev:pr/1208-reconciliation-scoping

Conversation

@rodboev

@rodboev rodboev commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

OpenCode roots that miss recursive watcher coverage currently enter the generic two-minute unwatched-root poller. Each wake runs authoritative root reconciliation and lists the shared SQLite archive, and a pass longer than the interval can consume an already queued wake immediately after completion. Large installations can therefore sustain archive-scale reads and CPU while idle.

This carries provider-owned coverage units from watch planning into native and degraded scheduling. OpenCode's SQLite unit uses the event journal to select a bounded batch of settled session IDs, resolves each ID through targeted metadata, and feeds the existing provider parse path. The coordinator permits one active pass per coverage key, coalesces wakes, and schedules the next pass from completion. Other providers retain provider-scoped reconciliation.

Journal rows, payload bytes, pending IDs, ready IDs, and pass time have explicit caps. Checkpoints advance only after archive writes, retain work committed during an audit, and require another audit when the SQLite container is replaced. Unsupported, removed, or oversized journal state requests one provider-scoped authoritative audit and latches repeated journal work. Missing probes, recursive symlink gates, shared generic watch roots, storage canonicality, persistent archives, startup repair, and global overflow recovery keep their existing authority. The change adds no database schema, mirror, frontend, or server API surface.

The watcher-exhaustion trace and production CPU/read measurements came from @berenddeboer's issue report.

Closes #1208

@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (14fd722)

Scope-bounding regressions remain in proof-scoped reconciliation.

Medium

  • internal/sync/engine.go:3627 — When the proof is an unconfigured descendant of a configured root, discovery processes all sources beneath the admitted ancestor. Sibling sessions outside the proof can be synchronized, and work scales with the ancestor’s entire corpus. Restrict discovery to the proof subtree where supported, filter candidates by physical source path, and test an out-of-proof sibling.

  • internal/sync/engine.go:4517 — An unrelated requested local root is appended to logicalRoots, preventing the no-op guard from firing. Reconciliation can then perform archive-wide subagent linking and persistence despite no matching provider. For agent-scoped reconciliation, omit roots unrelated to the agent’s configured roots so an empty scope returns immediately.


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

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 14fd722 to d989ab5 Compare July 30, 2026 03:14
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (d989ab5)

Code review found two medium-severity scope-bounding issues in reconciliation.

Medium

  • internal/sync/engine.go:4679 — Admitting the deepest configured ancestor still allows the provider to traverse and spool sources outside the proof root. For example, proof /root/team/project with configured root /root/team can also discover /root/team/sibling, potentially synchronizing or tombstoning out-of-scope members while work scales with the entire gateway subtree. Constrain traversal to the proof subtree, defensively reject candidates outside it before spooling, and add coverage for a sibling beneath the admitted gateway.

  • internal/sync/engine.go:3198 — A local root unrelated to all configured directories misses the intended no-op path because logicalRootsForAgentWatchRoots preserves unmatched roots. Reconciliation then runs without providers but still performs the archive-wide LinkSubagentSessions update, causing global work and potentially mutating unrelated sessions. Return before reconcileWatchRootsStreamed when no configured root overlaps the filtered request, and test that no global link pass or database mutation occurs.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m49s

@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (99ebf73)

Reconciliation has a medium-severity Hermes root-alias regression that can silently omit sessions and updates.

Medium

  • internal/sync/engine.go:3659, internal/sync/engine.go:4130 — Raw path containment does not preserve provider-defined root aliases. Hermes permits either sessions/ or state.db as an archive root while owning sources in both sibling locations. Reconciling a sessions/ root discards all state.db#id sources as outside scope.roots, and ownership narrowing removes the corresponding database scope; configuring state.db causes the inverse problem for transcript-only sources. Reconciliation still reports success despite missing data.
    • Suggested fix: Map archive-root aliases to provider-specific proof scopes for source admission and ownership boundaries. Add regression coverage for Hermes configured through both sessions/ and state.db.

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

@rodboev
rodboev marked this pull request as draft July 30, 2026 07:38
@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 99ebf73 to 48cb222 Compare July 30, 2026 07:38
@rodboev rodboev changed the title fix(sync): bound agent-scoped reconciliation to its requested roots fix(sync): bound agent-scoped reconciliation to provider-owned scopes Jul 30, 2026
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (48cb222)

The change needs fixes for three medium-severity reconciliation issues involving Hermes aliases, tombstone authority, and persistent archives.

Medium

  • internal/sync/engine.go:3214 — Mixed Hermes requests can skip archive reconciliation. Provider-resolved aliases are retained only when geometric matching finds no roots. For a request such as [/archive-a/state.db, /archive-b/sessions], the exact sessions match makes logicalRoots nonempty, causing the state.db alias to be discarded and archive A not to be reconciled. Preserve every provider-matched requested root or carry the resolved plan forward.

  • internal/sync/engine.go:4182 — Alias-based scopes can skip tombstone ownership paging. Completed scopes use RetryRoots for the configured-directory overlap gate. Because configured /archive/sessions and requested /archive/state.db are siblings, discovery succeeds but tombstone ownership paging is skipped, leaving removed archive members active. Keep retry roots separate from ownership identity and use the scope’s Identity or ProofScopes for tombstone authority.

  • internal/parser/hermes_provider.go:174 — Disappearing physical Hermes archives are not preserved. PersistentArchiveSource recognizes only virtual state.db#member paths, while normal SyncAll stores the physical aggregate state.db path on sessions. If that database disappears, reconciliation tombstones all associated sessions instead of preserving the persistent archive. Recognize validated physical state.db ownership as persistent and add coverage for a SyncAll-seeded archive whose database disappears.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 9m24s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 48cb222 to d82b807 Compare July 30, 2026 08:56
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (d82b807)

The reconciliation-scope changes have three medium-severity correctness regressions that should be addressed before merging.

Medium

  • internal/parser/hermes_provider.go:182PersistentArchiveSource cannot recognize physical state.db paths beneath a configured Hermes profiles container. Sessions imported by SyncAll retain the physical path; if the database disappears before streamed reconciliation rewrites it to a virtual path, reconciliation incorrectly tombstones the persistent archive. Validate physical state.db paths against profile children and missing archive roots without relying on the file-dependent hermesArchiveRootPaths.

  • internal/sync/engine.go:3817 — Generic reconciliation passes a requested descendant to the provider as its configured root. Root-relative providers such as Claude then discover from the wrong level, causing top-level session files to be skipped when reconciling a project beneath the configured projects directory. Preserve the provider’s required traversal root while enforcing the narrower path through proof filtering, or add provider-specific reconciliation plans for layout-sensitive providers.

  • internal/parser/hermes_provider.go:215 — The parent-directory comparison treats every direct sibling of state.db as an archive alias. Requesting an unrelated path under the archive directory can therefore reconcile the entire Hermes archive instead of producing a bounded no-op. Match only explicit aliases and artifacts, such as the archive directory, state.db, its WAL file, and the sessions directory.


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

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from d82b807 to d5778a3 Compare July 30, 2026 09:35
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (d5778a3)

Changes look sound overall, but one medium-severity Windows path-casing issue can cause Hermes reconciliation to silently no-op.

Medium

  • Case-sensitive proof admission conflicts with Windows path semanticsinternal/sync/engine.go:3773, internal/parser/hermes_provider.go:256
    Proof admission uses case-sensitive string matching even though reconciliation otherwise treats Windows paths case-insensitively. For example, requesting c:\sessions\project under configured root C:\sessions can silently discard discovered sources; case-variant Hermes state.db aliases may produce an empty plan. Use platform-aware path equivalence and containment for proof scopes and Hermes alias matching while preserving virtual-member semantics. Add tests where only the requested root’s casing differs.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 8m43s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from d5778a3 to 39dd34f Compare July 30, 2026 12:21
@rodboev rodboev changed the title fix(sync): bound agent-scoped reconciliation to provider-owned scopes fix(sync): bound OpenCode watcher fallback work Jul 30, 2026
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (39dd34f)

High-risk polling regressions remain: overlapping scopes can cause unintended tombstones, and OpenCode file-backed polling may leave updates stale.

High

  • cmd/agentsview/unwatched_poll.go:270 — Typed polling checks each obligation’s availability independently, bypassing cross-obligation probe gating. A present obligation can reconcile a shared or overlapping scope while another obligation’s physical subtree is missing, tombstoning sessions under that subtree.
    • Fix: Compute blocked and available scopes across the complete obligation snapshot before dispatching provider-specific work, preserving existing overlap rules.

Medium

  • internal/parser/opencode_provider.go:94 — Empty coverage keys are treated as OpenCode SQLite coverage, although OpenCode storage watch roots intentionally have no coverage key. If storage watching falls back to polling, the provider may try to open a nonexistent opencode.db instead of reconciling the storage tree, leaving updates stale.
    • Fix: Use bounded coverage only for explicit opencode-sqlite: keys; route empty or unrecognized keys through provider-root reconciliation.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 7m19s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 39dd34f to 9f5af1d Compare July 30, 2026 12:40
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (9f5af1d)

Medium-severity issues remain in path normalization and OpenCode change-feed timeout handling.

Medium

  • cmd/agentsview/main.go:2595syncDir preserves the unnormalized configured path, while the provider builds CoverageKey from a cleaned root. Paths such as /sessions/ fail the strict PollCoverage key check, causing archive-wide reconciliation on every event. Normalize configured roots before storing them in watchScope, or use the same canonical root when constructing and validating coverage keys.

  • internal/parser/opencode_change_feed.go:187 — A coverage timeout is ignored when rows.Err() coincides with ctx.Err(). The batch can be reported complete, its high-water state cleared, and no continuation requested, leaving unread events stale until another filesystem event. Return the context error or set More whenever the timeout occurs before LastRowID reaches HighWaterRowID.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 8m38s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 9f5af1d to 58ca00f Compare July 30, 2026 12:56
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (58ca00f)

The change needs fixes for two medium-severity watcher regressions before approval.

Medium

  • Rename events bypass bounded coveragecmd/agentsview/main.go:1907, cmd/agentsview/main.go:2356
    Coverage is derived only from batch.Paths, while rename paths are added afterward. Renaming or replacing opencode.db therefore falls back to changed-path discovery and enumerates every SQLite session. Route rename paths through coverage before dispatch, preserve consumed/excluded paths during rename handling, and add rename-specific regression tests.

  • Darwin watcher tests have stale scope expectationsinternal/sync/watch_backend_factory_darwin.go:64, internal/sync/watch_backend_factory_darwin.go:1065
    Fallback plans and polling obligations now contain scopes, but exact-equality tests still expect empty scopes, causing multiple Darwin watcher tests to fail. Update the expectations to include the appropriate WatchScope values and coverage keys.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 10m12s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 58ca00f to ff3a1f6 Compare July 30, 2026 13:13
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (ff3a1f6)

Review verdict: Three medium-severity issues require fixes before approval.

Medium

  • cmd/agentsview/main.go:2301 — Coverage-error retries preserve only batch.Paths, dropping coalesced ReconcileRoots, FullSync, and LostEvents work. Lifecycle tokens may then be acknowledged without the required authoritative reconciliation.

    • Fix: Preserve authoritative retry fields in both coverage-error paths, promoting to a full retry when FullSync is set.
  • internal/parser/opencode_provider.go:663 — The recursive storage/ watch root is registered only if the directory exists at startup. If created later under a SQLite-only root, nested session and message updates are missed until periodic sync or restart.

    • Fix: Register the missing root for lifecycle activation or dynamically add its recursive watch when the shallow parent observes its creation. Add a transition test.
  • internal/parser/opencode_change_feed.go:61 — Every OpenCode SQLite root advertises bounded coverage, including databases without the expected event schema. Those databases repeatedly return AuditRequired, causing archive-wide reconciliation after every poll.

    • Fix: Validate journal capability before assigning a coverage key, cache unsupported schemas, and use an explicit compatibility fallback.

Reviewers: 2 done | Synthesis: codex, 21s | Total: 9m24s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from ff3a1f6 to 3deffac Compare July 30, 2026 13:34
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (3deffac)

Changes requested: three medium-severity correctness issues could cause skipped reconciliation or duplicate OpenCode work.

Medium

  • cmd/agentsview/main.go:2327 — The code removes entries from paths while ranging over the same slice. Multiple exclusions can shift and skip entries, allowing them into normal changed-path discovery and causing duplicate, potentially archive-scale OpenCode work. Accumulate handled paths and filter after iteration; add a regression test covering both DB and WAL paths.

  • internal/parser/opencode_provider.go:717 — A SQLite-only root still receives a missing recursive storage watch root. During watcher failure, overlap gating between that missing probe and the valid SQLite probe blocks all polling obligations, so SQLite updates are never reconciled. Treat absent optional storage as lifecycle-only without blocking SQLite coverage, or gate bounded scopes using their own physical probe; test the complete SQLite-only watch plan under watcher failure.

  • cmd/agentsview/main.go:1923 — Generic ownership is checked only on the longest matching watch root. A bounded descendant under a generic ancestor can therefore be marked ConsumePath, preventing the ancestor provider from receiving the changed path. Select bounded coverage from the most-specific root while checking every matching root for generic scopes; add a nested overlapping-root test.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 9m48s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 3deffac to f41d684 Compare July 30, 2026 13:56
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (f41d684)

The PR has two medium-severity correctness and performance issues that should be addressed before merging.

Medium

  • Per-obligation pollability is lostcmd/agentsview/unwatched_poll.go:270
    When a present SQLite obligation and a missing NonBlockingProbe storage obligation share a sync root, the root is considered globally available and both obligations are dispatched. The missing storage obligation then falls back to full provider reconciliation, reintroducing archive-scale work on every fallback poll. Preserve probe availability per obligation while retaining cross-obligation blocking, and add a dispatch-level regression test for this case.

  • Continuation validation can miss journal replacement or compactioninternal/parser/opencode_change_feed.go:139
    Continuation pages validate only the last processed row, not the captured HighWaterEventID. If the processed row remains while the unread tail is replaced, validation succeeds and original tail events may be skipped, leaving stale sessions archived. On every continuation, verify that HighWaterRowID still maps to HighWaterEventID; trigger an audit if it is missing or changed.


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

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from f41d684 to 6439e2b Compare July 30, 2026 14:11
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (6439e2b)

Medium-severity issues remain in OpenCode coverage reconciliation.

Medium

  • internal/parser/opencode_provider.go:119 — The first coverage poll after every engine start always sets AuditRequired, triggering a full OpenCode archive scan even for an idle degraded poll. This violates the bounded background-work invariant and can repeatedly impose archive-scale work when daemons restart frequently. Initialize or persist the coverage checkpoint during successful startup reconciliation, reserve full audits for detected gaps or container replacement, and add an engine-level cardinality regression for this path.

  • internal/parser/opencode_provider.go:191 — Bounded coverage eligibility is fixed when the watcher plan is constructed. If the database or event table is absent, newly created, upgraded, or transiently unreadable at startup, CoverageKey remains omitted and subsequent database events use archive-wide changed-path discovery until restart. Retain a prospective coverage key and re-probe during dispatch, or refresh the watch plan when the database/schema appears; distinguish permanent incompatibility from transient probe failures.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 14m22s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 6439e2b to 8617ffb Compare July 30, 2026 14:41
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (8617ffb)

Changes requested: three medium-severity correctness and efficiency issues need attention.

Medium

  • internal/parser/opencode_change_feed.go:121 — The first degraded poll checkpoints the journal’s current maximum row without processing it. Changes occurring after startup reconciliation but before this poll—including deletions—are permanently skipped because degraded tasks have no ChangedPaths.

    • Fix: Establish the baseline before startup reconciliation, or perform an authoritative reconciliation on the first degraded pass before committing the high-water mark. Add a regression test for a change between startup sync and initial polling.
  • internal/parser/opencode_provider.go:122 — An unknown journal probe is treated as successful empty coverage. Native events for a missing or temporarily unreadable opencode.db are then consumed and excluded from generic changed-path handling, potentially leaving archived sessions active indefinitely after database removal.

    • Fix: Treat unknown support as retryable, or fall back to provider-root reconciliation instead of committing a successful checkpoint.
  • cmd/agentsview/unwatched_poll.go:274 — Typed scopes are dispatched independently for each polling obligation without cross-obligation deduplication. Multiple physical watch roots mapped to the same configured provider root can trigger duplicate archive-wide ReconcileProviderRoots calls every polling interval.

    • Fix: After applying probe gates, coalesce scopes by (Agent, Root, CoverageKey) across all obligations and dispatch each unique scope once.

Reviewers: 2 done | Synthesis: codex, 16s | Total: 9m36s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 8617ffb to 93fbe71 Compare July 30, 2026 15:04
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (2c58a73)

The change is generally sound, but one medium-severity symlink recovery issue can leave OpenCode data stale indefinitely.

Medium

  • cmd/agentsview/main.go:2908 — Recursive symlink handling drops NonBlockingProbe. A broken optional OpenCode storage symlink is consequently treated as blocking, preventing authoritative SQLite audits and leaving deletions or unknown-event changes stale indefinitely.
    • Fix: Preserve the nonblocking flag through providerSymlinkRoot, symlink polling obligations, and recovery-scope authority checks. Add a regression test covering a broken optional-storage symlink.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 10m38s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 2c58a73 to add0b84 Compare July 30, 2026 17:17
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (add0b84)

Medium-severity issues remain in the OpenCode change-feed handling.

Medium

  • internal/parser/opencode_change_feed.go:136 — Normal WAL checkpointing changes SQLiteContainerState while leaving MAX(rowid) unchanged, so the code treats routine checkpoints as corruption and triggers an archive-wide reconciliation, restoring unbounded work. Recognize same-container checkpoint transitions using journal anchors and advance the stored state without auditing; reserve full audits for replacement or invalidated anchors.

  • internal/parser/opencode_provider.go:146 — Every valid session.deleted.1 event sets AuditRequired, causing a full OpenCode archive scan for a single-session deletion. Carry removed session IDs through CoverageResult and tombstone those sessions directly; require full audits only when journal integrity is uncertain.


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

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from add0b84 to c41bab0 Compare July 30, 2026 17:57
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (c41bab0)

Verdict: Changes require fixes for one high-severity path traversal vulnerability and two medium-severity reconciliation regressions.

High

  • Directory traversal through OpenCode database identifiersinternal/parser/opencode_provider.go:837

    Database-controlled project_id and session IDs are passed directly to filepath.Join(sessionRoot, project, sessionID+".json"). Values containing ../ can escape sessionRoot, after which newSourceRef bypasses the existing containment checks. A crafted opencode.db could therefore cause agentsview to import and expose readable transcripts outside the configured storage root.

    Fix: Validate both identifiers as single path components, verify the cleaned path remains under sessionRoot using relUnder, and construct it through sourceRef instead of newSourceRef. Consider symlink containment if an attacker can write storage directories.

Medium

  • Incomplete storage-shadow resolutioninternal/parser/opencode_provider.go:826

    Shadow resolution checks only global and the SQLite project_id, while existing behavior allows any same-ID storage JSON to shadow SQLite even when its directory differs from that project ID. Bounded coverage can consequently parse stale SQLite data and hide a valid storage-backed session.

    Fix: Preserve provider-wide same-ID shadow resolution using a bounded storage index, or request an authoritative audit when ownership cannot be determined safely.

  • Coverage deletions bypass archived-session filteringinternal/sync/engine.go:3332

    Coverage deletion events tombstone sessions without applying missingMemberTombstoneAllowed. Narrowing the CWD filter should freeze archived, out-of-scope sessions, but this path deletes them.

    Fix: Check missingMemberTombstoneAllowed before tombstoning and add a filtered-deletion regression test.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 9m32s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from c41bab0 to 2abf622 Compare July 30, 2026 18:35
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (2abf622)

The PR needs changes for two medium-severity correctness issues in polling coverage and deletion-state handling.

Medium

  • cmd/agentsview/unwatched_poll.go:271 — Typed coverage tasks are incorrectly filtered through globally available reconciliation roots. If a blocking sibling probe such as OpenCode’s storage root is unavailable, the shared root is removed and the bounded SQLite journal is not polled, despite being safe with AuthoritativeFallback=false.

    • Fix: Gate keyed coverage feeds on their own probe/root availability. Use the global probe set only to determine fallback authority and gate generic or provider-wide reconciliation.
  • internal/parser/opencode_change_feed.go:240 — Removal state follows settled rather than actual source existence. A later pending message event clears RemovedIDs, potentially losing a tombstone, while a later session.created or session.updated event leaves removal set, causing a recreated session to be synchronized and then immediately tombstoned.

    • Fix: Track deletion state explicitly. Clear it only when an event proves the session exists again, preserve it across message and part events, and add tests for mixed delete/update sequences.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 10m36s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 2abf622 to 38d5abc Compare July 30, 2026 19:04
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (38d5abc)

Medium

  • internal/sync/engine.go:3250 — Coverage reconciliation is not serialized per coverage key. Concurrent watcher and polling calls can read the same checkpoint and apply effects out of order before the conflict is detected during CommitCoverage. A newer recreation may be committed before an older deletion tombstones it; the older commit then fails after the side effect, leaving the session incorrectly tombstoned. Hold a per-agent/coverage-key lock across the full poll, sync/tombstone or audit, and commit sequence, and add a concurrency regression covering deletion followed by recreation.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 11m37s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 38d5abc to 9d14f2a Compare July 30, 2026 19:27
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (9d14f2a)

High-severity watcher panic and a medium-severity archive overwrite risk need fixes before merge.

High

  • cmd/agentsview/main.go:1855 — Daemon archive watchers pass a nil *sync.Engine as activeSourceProbe. The typed nil becomes a non-nil interface, so certain OpenCode SQLite events invoke HasActiveSessionSourceBelow on a nil engine and panic the watcher.
    • Fix: Pass an explicitly nil interface when engine == nil, and add a regression test dispatching an OpenCode DB/WAL event through the nil-engine watcher.

Medium

  • internal/parser/opencode_provider.go:874coverageStorageIndex treats a temporarily missing storage subtree as definitively absent. When fallback authority is unavailable, a SQLite virtual source can overwrite a preserved storage-backed archive session.
    • Fix: Defer coverage when storage shadowing cannot be proven, or verify the archived session’s source ownership before replacement.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 11m12s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 9d14f2a to 43dc456 Compare July 30, 2026 20:19
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (43dc456)

Medium-severity issue found in OpenCode deletion reconciliation; no Critical or High findings.

Medium

  • internal/parser/opencode_provider.go:170 — A deletion event without a live storage shadow always emits the SQLite virtual path. If the archived session was owned by a now-deleted storage JSON file, tombstoneCoverageSessions rejects the mismatched path but still commits the journal checkpoint, leaving the session active until a later full reconciliation.
    • Fix: Resolve and tombstone the archived missing ownership, or require an authoritative audit when the archived path differs from the virtual removal path. Add coverage for simultaneous deletion of a storage-backed session’s JSON file and SQLite row.

Reviewers: 2 done | Synthesis: codex, 12s | Total: 10m50s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 43dc456 to 07c52e8 Compare July 30, 2026 20:44
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (07c52e8)

The change is generally sound, but two medium-severity watcher retry and coverage-routing issues should be addressed.

Medium

  • Retry drops rename informationcmd/agentsview/main.go:2444, internal/sync/watcher.go:1221
    If coverage handling fails before rename processing, the retry includes Renames, but callbackRetryBatch discards them. A coalesced directory or ambiguous rename may then be retried only as a changed path and never receive authoritative reconciliation. Preserve renames through retry retention, or promote retries containing directory/ambiguous renames to a full reconciliation. Add a watcher-level regression test for a coverage failure coalesced with a directory rename.

  • Header-only WAL events trigger unnecessary workcmd/agentsview/main.go:2107
    Coverage routing matches every opencode.db-wal event by glob and bypasses sqliteWALHasFrames, which intentionally ignores missing or header-only WALs created by read-only SQLite access. Because coverage polling opens the database, these events can repeatedly wake the watcher and trigger unnecessary coverage and archive-push work. Restore the WAL-frame predicate for native coverage routing, such as by making PollCoverage ignore WAL-only ChangedPaths without transaction frames.


Reviewers: 2 done | Synthesis: codex, 11s | Total: 11m12s

@rodboev
rodboev force-pushed the pr/1208-reconciliation-scoping branch from 07c52e8 to 5fac113 Compare July 30, 2026 21:10
@roborev-ci

roborev-ci Bot commented Jul 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (5fac113)

Medium severity issue found; no High or Critical findings.

Medium

  • Unbounded archive reconciliationinternal/parser/opencode_provider.go:189
    Exceeding the fixed storage-shadow probe budget sets AuditRequired, causing ReconcileCoverage to enumerate the entire hybrid storage archive. Large archives may therefore trigger archive-scale work for every SQLite event, defeating the bounded watcher path.
    Fix: Resolve storage ownership through a persistent or resumable bounded index, and add a small-versus-large hybrid archive regression proving per-event work remains bounded.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m41s

@berenddeboer

Copy link
Copy Markdown
Contributor

Watching this with great interest @rodboev . My two cents is that I probably would have done a much purer queuing implementation. Watchers/pollers who dump work in the queue, and job workers who work on the queue, and probably have done this through a db queue, instead of in memory. If you are interested, you can see that implemented in ready-for-agent.

So I have a job queue, implementing fairly standard queue semantics, and rely on db locking for queue semantics. Queue implementation is an Effect TS service.

I suppose this isn't easy to read if you don't know TypeScript or use Effect TS (that's FP for TypeScript).

But I've found this pattern to be very stable, and survives restarts, which an in-memory implementation doesn't. For agentsview that's not needed, but still nice to have fast starts.

@rodboev

rodboev commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Agreed that the OpenCode journal feed should be a pure package. I am thinking of a split along these lines, listed in merge order; let me know what you think:

  • D: Watch split. A non-recursive watch on the configured root for opencode.db and its WAL, exempt from the recursive budget, plus a recursive watch on storage/ when that layout resolves. Keeps a budget-starved root natively watched rather than on the two-minute poller. Registration starvation only; overflow, unavailable backends, and poll cost stay with the later slices.
  • A: Reconciliation-scoping half promised in #1262 (comment): a reconcile pass walks only the roots it was asked about, with normalization done once through the provider contract.
  • B: OpenCode journal feed as a pure package: journal rows in; ready and pending session identities out, plus continuation state and a latched audit reason. One contract change from the version here: the feed no longer resolves final sources. (Several findings here traced back to one provider trying to reconstruct archive ownership from the filesystem when the engine already owns that through shared stored paths and the existing FindSource path, so resolution moves engine-side.)
  • C: Integration, stacked on fix(daemon): scope degraded-coverage polling to one provider per pass #1307 and A + B: the engine resolves feed identities against archive ownership through the existing FindSource path, checkpoints commit only after the archive write and are serialized per coverage key, archive-scale reconciliation is gated behind the audit scheduler, and a keyed worker owns admission, wake coalescing, and one active pass per coverage unit.

B and C ship as one PR, since C is B's only consumer.

C is essentially the queue model you're suggesting: watchers and pollers enqueue keyed work, serialized workers apply it and ack after the archive write. In-process still makes more sense than db-backed, since OpenCode's journal is already the durable log and startup reconciliation the recovery path; persisting the queue would just add a second acknowledgement protocol.

The Effect TS service isn't importable from Go, but the semantics (unique keyed jobs, transactional claiming) are good reference points. Thanks for pointing that out.

@rodboev rodboev closed this Jul 30, 2026
@rodboev

rodboev commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

It looks like #1291 takes out most of the reason for B and C; once it's merged, unchanged sessions will get skipped, and an idle pass over a large container becomes cheap.

So based on that:

@berenddeboer

Copy link
Copy Markdown
Contributor

@rodboev looking at #1291 I feel it gets into the same issue I ran into with my third implementation. The code review never converges, and the code writing agent never solves all the problems you still have as its an architectural issue.

My hunch is that your first slice also won't converge, as you than run into the 2 minute poll / max time (I can't exactly describe the issue, but it feels this is what I still ran into).

So I would have pure work generators, which read the two opencode histories I know about (the old one and the new .db one), but do no work. They simply write session ids or session id ranges. These work generators keep scanning or polling. Then the job workers receive a range of session ids and read/parse them, and put them into agentsview. One can control the cpu/load of a session parser by the number of sessions it does at a time. Although for super large sessions this could still be substantial.

My first slice: write the work generators, queue and job worker purely for the old style opencode sessions, so that work gets removed from the normal path. If roborev is happy with this slice, the second step would be to move the modern opencode v1 db into this architecture.

The advantage of slice 1 is that this is old style, so you don't need to write the code to keep rescanning/polling this, we can be sure this is just importing old history. For slice 2 you then add the continual scanning for updates, but you have the queue architecture already in place.

Again I'm just sketching here, I don't know the agentsview code base nor the opencode architecture too well, this is just based on seeing my clanker attempting to solve this three times and running into one issue after another

@berenddeboer

Copy link
Copy Markdown
Contributor

@rodboev what I might do is just let the clanker write up a description of the two opencode architectures.

On the size mentioned in #1291 : my opencode is 27GB, so double.

@berenddeboer

Copy link
Copy Markdown
Contributor

@rodboev : I let the clanker write up a description of opencode's storage format. I think it's more useful detail than session-format-sources.md, but not sure you want this in your docs so haven't created a PR for this.

And the Xplain db format is extremely concise, and I can read this extremely fast and well, but it's very niche. But because it hides the details I find it great for humans.

I think this explains my hunch for slice 1 well: drop the json reading from the current path, do it via the new queue mechanism, and then you're done forever (except when you request a full scan).

Slice 2 is then fixing the issue that changing one session causes an entire repass. And hopefully this can fit nicely onto the new queue.

Please let me know if you want me to help out anywhere.

@rodboev

rodboev commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

I think the 2 minute poll / max time is in cmd/agentsview/main.go (unwatchedPollInterval = 2 * time.Minute), and #1208 diagnoses it correctly: a poll took longer than two minutes, so the queued ticker wake started the next one immediately and the daemon had no idle interval.

I added that slice as D above. It lands ahead of A and keeps a budget-starved root off the poller; completion-based scheduling is in C.

The writeup is useful, thanks. Backfilling the JSON tree and treating the root as SQLite-only works makes sense for OpenCode, but I don't think it extends to the forks: my Kilo and MiMoCode ship the journal tables empty and still write storage/ live, so they keep needing the recursive watch.

The backfill also flips hybrid shadowing, since the DB row would win where JSON does today. That plus the per-root state can be a follow-up slice; I'm trying to keep this reviewable.

@berenddeboer

Copy link
Copy Markdown
Contributor

Yes good point, db backfill shouldn't overwrite json. But implicitly I also wanted to improve the startup time of agentsview, so it doesn't start with a full scan, this simply gets scheduled when the parsing of all agents move to this architecture.

My opencode is 27GB now, and I'm getting better at this stuff, aiming for a consistent 150+ PRs a week, so startup time is going to be a real issue.

@rodboev

rodboev commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Yeah, I'm in the same boat with tens of GB of DBs spread across various harnesses. I would also like to see it queue things up in the background. The desktop app is much slower than having agentsview serve in the background on login and hit the web app on demand. There's still a quick reconciliation but much faster than desktop startup, so I avoid the app for now. Not sure which lag you're referring to, but try that if you haven't.

I've slowed down but my rate was about the same at the 30-day mark with around 500 opened. I manually trim history to last 2 weeks to keep it manageable between agentsview and my hand-rolled history viewer. Sometimes this is the reason I use the hand-rolled one though — it caches on startup, but I also don't particularly care about the RAM hit.

@berenddeboer

Copy link
Copy Markdown
Contributor

I run it with agentsview serve. Just doesn't work. But my history goes all the way back to aider days. There's a lot. And I feel agentsview can potentially give me a lot of valuable data, and simply tracking improvements over time is already valuable.

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.

fix(sync): bound OpenCode background work across watcher and polling modes

2 participants