Skip to content

fix(cache): serve published analytics while refresh runs - #914

Merged
wesm merged 8 commits into
kenn-io:mainfrom
salmonumbrella:fix/analytics-cache-rebuild
Sep 27, 2026
Merged

wesm merged 8 commits into
kenn-io:mainfrom
salmonumbrella:fix/analytics-cache-rebuild

Conversation

@salmonumbrella

@salmonumbrella salmonumbrella commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Archive queries read the last published analytics cache while a refresh builds in the background. Responses report the generation, publication time, staleness, pending additions, and build state.

  • CLI queries wait when a cache build is required, then return rows or an error. --fresh includes writes committed before the request; if a build is already running, callers receive the queued follow-up job's ID. HTTP and MCP callers can poll that job and repeat the query.
  • Automatic refreshes honor auto_build_cache, min_rebuild_interval, and retry delays. Deleted rows can remain visible until the replacement is published. Manual sync accepts --build-cache and --no-build-cache and leaves full cache checks to the background worker. Explicit cache flags for sync and sync-full require daemon API schema 2.31.0.
  • Adds advanced MCP query_sql through a dedicated DuckDB instance confined to archive analytics files. MCP advertises it only for supported SQLite daemons. Restricted SQL setup and queries use the request context. Owner CLI and HTTP SQL retain their existing capabilities.

API schema 2.31.0 and generated clients cover freshness metadata, accepted builds, job status, and restricted archive SQL. The daemon retains the last 100 completed jobs plus active work; restarting clears that history. A completed check can reuse an unchanged publication.

Includes test timing fixes for Windows lifecycle checks and the timing lint rules.

Refs #912

@roborev-ci

roborev-ci Bot commented Sep 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (4688ce0)

Verdict: Changes require fixes for 2 findings.

Medium

  • cmd/msgvault/cmd/serve.go:1975: The generic CLI proxy schedules a cache refresh for every listed sync command, including sync-circleback --probe and sync-notion-meetings --probe, even though probe modes only validate access and perform no archive writes. Detect probe mode before registering the deferred refresh, or have the subprocess report whether it actually wrote archive data before queuing a build.

    Reported by: codex

  • cmd/msgvault/cmd/cache_jobs.go:97: Cache jobs run in an untracked goroutine, so daemon shutdown cancels the context without waiting for execute/cmd.Run to finish before closing the archive and returning. A build may still be terminating during cleanup or restart, leaving locks or staged cache files in an indeterminate state. Track active cache jobs with a wait group and wait for them after cancellation, bounded by the shutdown deadline, before closing the store.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 11m10s

@roborev-ci

roborev-ci Bot commented Sep 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (bd30f40)

Verdict: Changes require fixes for 2 findings.

Medium

  • cmd/msgvault/cmd/cache_jobs.go:85: A cache build request arriving while another build is active is coalesced into the current job and discarded. A manual sync can commit new messages during the build, allowing it to publish an outdated snapshot without queuing a follow-up build. Track a pending follow-up refresh when writes occur during an active build, or recheck staleness after completion and enqueue another build before marking the refresh settled.

    Reported by: codex

  • cmd/msgvault/cmd/cache_jobs.go:44: verifyWhenDue records lastVerification before the asynchronous scheduled build runs but never clears it when the build fails. For conversation-only drift, ordinary queries can suppress automatic verification for the full interval while serving stale data. Reset or otherwise invalidate lastVerification when a scheduled verification job finishes unsuccessfully, preferably with bounded retry/backoff.

    Reported by: codex


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

@salmonumbrella
salmonumbrella force-pushed the fix/analytics-cache-rebuild branch from bd30f40 to b838c0e Compare September 23, 2026 11:47
@roborev-ci

roborev-ci Bot commented Sep 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (b838c0e)

Verdict: Changes require fixes for 3 findings.

High

  • internal/mcp/handlers.go:161: The read-only query_sql MCP tool allows SELECT statements using DuckDB file-reading functions such as read_blob, read_text, or read_csv_auto, enabling authenticated clients to read arbitrary host files. Remove arbitrary SQL exposure or enforce an allowlist of archive analytics views and reject external file/table functions; alternatively use a restricted DuckDB connection with external access disabled.

    Reported by: codex (security)

Medium

  • internal/query/duckdb.go:303: DuckDBEngine.QuerySQL reads the publication marker and executes Parquet-backed SQL without the shared cache read lock or view refresh, allowing concurrent publication to produce mixed, removed, or stale cache state. Acquire e.acquireCacheRead(ctx) for the full QuerySQL operation so the committed generation is validated and optional columns/views are refreshed before execution.

    Reported by: codex

  • cmd/msgvault/cmd/serve.go:1085: Failed scheduled automatic builds set verificationRetryAt, but the stale-cache query path accepts scheduled automatic builds without checking that backoff, potentially launching a subprocess on every query after a persistent failure. Apply the retry cooldown before accepting scheduled automatic builds in the staleness path, or centralize scheduled-job acceptance so all automatic retry paths honor verificationRetryAt.

    Reported by: codex


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

@wesm wesm self-assigned this Sep 23, 2026
@roborev-ci

This comment has been minimized.

@wesm

wesm commented Sep 23, 2026

Copy link
Copy Markdown
Member

What trust level do you intend consumers of the new query_sql MCP tool to have?

The project treats CLI and HTTP API users as privileged, so file-reading SQL alone does not establish a vulnerability under that threat model. Extending that capability to MCP does raise a design question about consumers with different levels of trust.

A synthetic test against this PR confirmed that an MCP-only key can read an outside-archive file through read_text or read_blob, with MCP writes disabled. The same key was denied direct access to daemon SQL; MCP used its own daemon credentials. The MCP guide currently says it cannot read Google credentials.

Would you prefer to keep SQL as an explicitly trusted advanced capability, make it opt-in or restricted for less-trusted MCP consumers, or move it to a separate PR? This seems worth an explicit decision before changing the tool or treating the review's severity as settled.

@salmonumbrella

Copy link
Copy Markdown
Contributor Author

@wesm you’re right. The goal of #912 and this PR is for archive queries to return promptly from a published cache generation, report freshness, and let callers request a background refresh. query_sql gives MCP clients that query path; it is not meant to give an MCP-only read-only key access to arbitrary host files.

I’d keep the MCP SQL tool, but restrict its DuckDB connection to archive analytics data, with external file access disabled. CLI and HTTP can retain their existing privileged behavior. If we cannot make that boundary reliable in this PR, I’d move the MCP tool to a follow-up rather than ship the file access. Your synthetic test makes this an actionable MCP trust-boundary issue.

@wesm
wesm force-pushed the fix/analytics-cache-rebuild branch from a76eee8 to f3f43fb Compare September 25, 2026 21:35
@roborev-ci

This comment has been minimized.

wesm added a commit to salmonumbrella/msgvault that referenced this pull request Sep 25, 2026
MCP clients receive archive access without inheriting the daemon owner's
ability to read arbitrary host files. Use a separate DuckDB instance with
native file restrictions and locked configuration. A dedicated endpoint
also prevents an older daemon from silently ignoring the restriction.
Owner CLI and HTTP queries retain their existing capabilities.

The archive endpoint shares publication locking, refresh jobs, query
limits, and read access during other archive work. Document the new MCP
tool and publish its generated API contract.

Also remove competing timers from the request-duration policy test. Under
CI load both timers could be ready, making a correctly bounded request
randomly appear unbounded; the test now waits for actual cancellation.

Addresses the MCP SQL finding in PR kenn-io#914 review:
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

wesm added a commit to salmonumbrella/msgvault that referenced this pull request Sep 26, 2026
MCP clients receive archive access without inheriting the daemon owner's
ability to read arbitrary host files. Use a separate DuckDB instance with
native file restrictions and locked configuration. A dedicated endpoint
also prevents an older daemon from silently ignoring the restriction.
Owner CLI and HTTP queries retain their existing capabilities.

The archive endpoint shares publication locking, refresh jobs, query
limits, and read access during other archive work. Document the new MCP
tool and publish its generated API contract.

Also remove competing timers from the request-duration policy test. Under
CI load both timers could be ready, making a correctly bounded request
randomly appear unbounded; the test now waits for actual cancellation.

Addresses the MCP SQL finding in PR kenn-io#914 review:
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the fix/analytics-cache-rebuild branch from ad5d5a0 to 881b9d8 Compare September 26, 2026 14:42
@roborev-ci

This comment has been minimized.

wesm added a commit to salmonumbrella/msgvault that referenced this pull request Sep 26, 2026
PostgreSQL daemons report the current API schema but cannot run archive
SQL. Use their existing health metadata to omit query_sql from MCP tool
discovery instead of advertising a tool that always fails.

Opening the restricted DuckDB instance used a background context, so a
request could expire while setup still waited for cache publication.
Carry its context through initialization, schema probes, view setup,
and access restrictions. A canceled refresh must not mark incomplete
schema state as current.

The nested query() finding in the same review is not an issue. Real
DuckDB 1.5.4 tests reject mutating and multiple statements inside query(),
and nested file reads remain subject to the archive restrictions.
Review: kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

wesm added a commit to salmonumbrella/msgvault that referenced this pull request Sep 26, 2026
Older supported daemons ignore the new cache parameters and report a
successful sync, even when the caller requested --no-build-cache.
Check the advertised API schema before starting either sync mode with
an explicit cache flag. Report an upgrade error when the daemon cannot
honor it. Ordinary sync keeps its existing compatibility.

Addresses the verified sync flag finding in PR kenn-io#914:
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

salmonumbrella and others added 8 commits September 26, 2026 16:40
The Windows CLI shard timed out waiting for the helper child PID before
it reached the process-tree termination check. Child startup within ten
seconds is not part of the shutdown contract.

Use the existing bounded lifecycle readiness wait in both Windows tests.
Keep the process containment and termination checks unchanged.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
MCP clients receive archive access without inheriting the daemon owner's
ability to read arbitrary host files. Use a separate DuckDB instance with
native file restrictions and locked configuration. A dedicated endpoint
also prevents an older daemon from silently ignoring the restriction.
Owner CLI and HTTP queries retain their existing capabilities.

The archive endpoint shares publication locking, refresh jobs, query
limits, and read access during other archive work. Document the new MCP
tool and publish its generated API contract.

Also remove competing timers from the request-duration policy test. Under
CI load both timers could be ready, making a correctly bounded request
randomly appear unbounded; the test now waits for actual cancellation.

Addresses the MCP SQL finding in PR kenn-io#914 review:
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
A query that needed a cache build could exit successfully without rows.
The CLI now waits for the accepted job and repeats the query. Fresh requests
queue a later snapshot when a build is already running, so their job IDs
cover writes committed before the request.

Keep automatic work bounded by retry delays and retained job history.
Startup owns its recovery, and completed syncs queue full checks in the
background without turning daemon shutdown into a sync failure. Use a
read-only store for staleness checks so closing a query probe does not
wait for a WAL checkpoint behind the builder's snapshot.

Remove the unused SQL runner, align freshness documentation, and expose
MCP SQL only on daemons with the restricted endpoint. Preserve normal
200 responses in both generated SQL convenience methods. Replace short
startup timers and elapsed-time assertions with bounded lifecycle waits
and checks against a held builder snapshot.

The generic sync regression substitutes only the external child runner;
it exercises actual flag parsing, cache state, and job acceptance without
requiring provider credentials or a live service.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
Local testify helpers already carry the test handle. Passing it again
made the query codegen regression compare the wrong values after the
helper conversion.

The upload deadline starts before database reads and local preparation.
Under CI load its first attempt can time out before reaching the test
server, so two server-observed requests are not guaranteed. Keep the
pending timeout, successful size allowance, and single-upload checks.
The restart test already checks replay with the same operation ID after
a server-side disconnect.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
PostgreSQL daemons report the current API schema but cannot run archive
SQL. Use their existing health metadata to omit query_sql from MCP tool
discovery instead of advertising a tool that always fails.

Opening the restricted DuckDB instance used a background context, so a
request could expire while setup still waited for cache publication.
Carry its context through initialization, schema probes, view setup,
and access restrictions. A canceled refresh must not mark incomplete
schema state as current.

The nested query() finding in the same review is not an issue. Real
DuckDB 1.5.4 tests reject mutating and multiple statements inside query(),
and nested file reads remain subject to the archive restrictions.
Review: kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
Older supported daemons ignore the new cache parameters and report a
successful sync, even when the caller requested --no-build-cache.
Check the advertised API schema before starting either sync mode with
an explicit cache flag. Report an upgrade error when the daemon cannot
honor it. Ordinary sync keeps its existing compatibility.

Addresses the verified sync flag finding in PR kenn-io#914:
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
The timing check enabled on main rejects two existing waits. Let the
successful rerank request finish accounting before a parallel failure
cancels the group, using synctest instead of polling the budget. The
worker test already observes its startup marker before cancellation;
the extra delay establishes no additional lifecycle state. Reap the
child on assertion failures as well as on the successful test path.

The two compatibility findings in the review below need no changes.
Supported older query daemons rebuild stale caches synchronously or
return an error. Generic sync commands already reject unsupported flags
before starting work. Both findings were accepted as not issues.
kenn-io#914 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the fix/analytics-cache-rebuild branch from 8c7590e to 8c34c02 Compare September 26, 2026 21:48
@roborev-ci

This comment has been minimized.

@wesm
wesm merged commit 8b0a966 into kenn-io:main Sep 27, 2026
46 of 47 checks passed
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.

2 participants