feat(cache): implement per-table tagging and raw SQL bypass

[taitelee]

Summary

This PR implements the Read-Your-Writes architectural consensus established in #73. It addresses the issue of stale data in the API query cache by introducing a granular, table-based invalidation signal triggered by the ingest pipeline.

• Option C (Raw SQL Bypass): Stripped caching from the raw SQL path (/v1/query). Admin and ad-hoc queries now hit ClickHouse directly to ensure absolute consistency for manual debugging and E2E tests.
• Option B (Table Tagging): - Updated the Cache interface to support tagging.
  • Implemented a tagsMap in LocalCache to track which cache keys are associated with specific database tables.
  • Added naive SQL parsing in pipes.go and table-name extraction in structured_query.go to tag cache entries during the Set operation.
• Ingest Integration: Wired the TieredCache into the Bento ingest worker. Upon a successful batch flush to ClickHouse, Bento now triggers a targeted invalidation of all cache entries tagged with that table name.

Test plan

• Unit Tests: Updated internal/cache tests to cover tag-based eviction and verified signature consistency across mocks.
• Manual Verification: Verified that raw SQL queries return X-Cache: BYPASS headers.
• E2E Integration: Verified that a structured query performed immediately after a Bento flush returns fresh data rather than a stale cached result.

Related Issues

Closes #73, #85, #93

Summary by CodeRabbit

• New Features

  • Cache tagging for table-scoped entries; automatic per-table invalidation after successful DB writes
  • Query endpoint will explicitly indicate cache bypass via X-Cache: BYPASS

• Breaking Changes

  • Cache write/invalidation API updated to support tags
  • /v1/query now bypasses cache by default

• Bug Fixes

  • Fixed TOCTOU race in local cache invalidation

• Documentation

  • Updated API and architecture docs for tagging, invalidation, and cache header behavior

• Tests

  • Added tests for tag extraction and tag-based invalidation

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a robust mechanism to maintain data consistency between the API cache and the underlying ClickHouse database. By implementing table-based tagging, the system can now selectively invalidate stale cache entries when data is ingested. Additionally, it addresses consistency concerns for raw SQL queries by bypassing the cache entirely, ensuring that manual and automated debugging tasks always reflect the current state of the database.

Highlights

• Table-Based Cache Invalidation: Implemented a tagging system in the cache layer, allowing for granular invalidation of cache entries associated with specific database tables.
• Raw SQL Cache Bypass: Disabled caching for raw SQL queries (/v1/query) to ensure absolute data consistency for administrative and debugging operations.
• Ingest Pipeline Integration: Integrated the cache invalidation logic into the Bento ingest worker, ensuring that cache entries are purged automatically after successful batch flushes to ClickHouse.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a tag-based cache invalidation system to ensure data consistency when tables are modified. It updates the Cache interface with InvalidateByTags, implements this in LocalCache using a tagsMap, and integrates invalidation logic into the PipesHandler, QueryHandler, and StructuredQueryHandler. Additionally, the ingest worker now triggers invalidation upon writing batches. Feedback focuses on a potential race condition in InvalidateByTags that could lead to orphaned cache entries and suggests refactoring duplicated table extraction logic into a shared, validated helper to align with repository security standards.

[gemini-code-assist]

Code Review

This pull request introduces tag-based cache invalidation to ensure consistency between ClickHouse and the API cache. Key updates include modifying the Cache interface to support tags, implementing a tag-to-key index in LocalCache, and integrating invalidation triggers within the ingestion pipeline and raw SQL query handler. Feedback highlights a high-severity race condition in the InvalidateByTags implementation that could result in stale cache entries and recommends refactoring duplicated table extraction logic into a shared helper that enforces strict identifier validation. Iterate.

[claude]

2 [MUST], 2 [SHOULD], 0 [MAY] — see inline threads for detail.

Verdict: Iterate.

The implementation direction is sound and the Gemini-flagged race from the prior commit was correctly addressed via LoadAndDelete. Two blockers remain:

1. Residual TOCTOU in InvalidateByTags (local.go:71) — LoadAndDelete closes the "append-during-Range" window but opens the symmetric one: a concurrent Set that runs after the delete creates a fresh *sync.Map that the current pass never sees, leaving pre-ingest data cached until TTL. Switch tagsMap to a plain map guarded by a sync.RWMutex to make the delete and any racing Set mutually exclusive.
2. Missing documentation sync (cache.go:17) — docs/api.md (X-Cache: BYPASS), docs/architecture.md (interface change), and CHANGELOG.md ([Unreleased] entry) are all required by AGENTS.md and absent from this diff.

The tagsMap memory leak ([SHOULD], local.go:63) and too-coarse test mocks ([SHOULD], mocks.go:147) should be addressed in this PR as well — the mocks as written can hide L1/L2 routing bugs in TieredCache.

[claude]

Review summary

1 [SHOULD], 3 [MAY], 0 [MUST] — see inline threads for detail.

The core implementation is solid: the double-indexed tag map (tagsMap / keyTags) with version-stamped eviction callbacks is the right approach to the TOCTOU race that a plain sync.Map couldn't handle. Wait() before the invalidation sweep correctly drains Ristretto's async admission buffer. The regex pipeline (strip literals → strip quoted identifiers → mutationRe) is correct and the \b anchors prevent false positives on column names like drop_rate. Integration tests exercise the full RYW path through the real HTTP layer. Doc sync is complete.

Threads:

• query_test.go:228 — [SHOULD] misleading test name + missing mutationRe assertion for unquoted DML-prefixed column names
• pipes.go:167, structured_query.go:161 — [MAY] setCancel() should be defer setCancel()
• query_test.go:355 — [MAY] REPLACE INTO not covered in TestExtractMutationTargets

Ship it — address the [SHOULD] test gap; the [MAY] nits can go in a follow-up if preferred.

[claude]

[SHOULD] Security: NONE / Correctness: LOW — Residual TOCTOU in the expiry eviction path.

The prior round's taitelee reply (3231258193) stated "once the tagsMu.Lock() is acquired, we verify again the TTL status. If a concurrent update happened, we gracefully abort the deletion and return the fresh value." But the code does not implement that re-check. The race scenario is:

1. Get calls l.ttls.Load(key) (line 110) and reads the old expiry.
2. Concurrent Set acquires tagsMu.Lock(), writes a new value + new TTL into l.ttls, updates keyVersion, releases lock.
3. Get computes remaining <= 0 (true based on the old expiry), acquires tagsMu.Lock().
4. Get deletes keyVersion[key], removes tags, and calls l.cache.Del(key) — all of which were just written by the concurrent Set.

Result: a live, freshly-written entry is silently evicted. The next caller gets a spurious MISS and re-queries ClickHouse — no data loss, but the invariant the author described is not actually enforced.

Fix: re-read and re-check the TTL inside the lock before deleting:

Suggested change

	l.tagsMu.Unlock()
	// Re-check TTL inside the lock: a concurrent Set may have
	// refreshed the expiry between our outer check and here.
	if freshExpVal, freshOk := l.ttls.Load(key); freshOk {
	freshExp := freshExpVal.(time.Time)
	if !freshExp.IsZero() && time.Until(freshExp) > 0 {
	// Entry was refreshed; return the live value.
	l.tagsMu.Unlock()
	return val.data, time.Until(freshExp), nil
	}
	}
	l.cache.Del(key)
	l.ttls.Delete(key)
	delete(l.keyVersion, key) // Clean up version tracker
	l.removeKeyFromTagsLocked(key)
	l.tagsMu.Unlock()
	return nil, 0, nil

[claude]

[SHOULD] Correctness: LOW — registerOnce (the package-level sync.Once at line 49) captures apiCache from the first StartIngestWorker call. Any subsequent call with a different apiCache silently discards its argument: the Bento plugin factories close over the original value.

The integration test documents this as "non-negotiable" (setup_test.go lines 10-13) and cmd/wavehouse/main.go only calls StartIngestWorker once, so there is no current regression. However the API is a silent trap — a future hot-reload handler, a second integration suite, or any caller that passes a fresh cache will observe stale invalidations with no error.

Minimum fix: add a godoc comment on StartIngestWorker stating the one-call-per-process constraint, consistent with the constraint the integration test already documents internally:

Suggested change

	cache cache.Cache
	cache cache.Cache // captured in the registerOnce factory; see StartIngestWorker godoc

And on StartIngestWorker:

// StartIngestWorker may only be called once per process: Bento's
// service.Register* functions are process-global singletons (guarded by
// registerOnce). The apiCache and chConn arguments are captured in the plugin
// factory closures on the first call; subsequent calls reuse those captured
// values regardless of the arguments passed.

[claude]

Review — feat(cache): per-table tagging + raw SQL bypass (round 9)

0 [MUST] · 2 [SHOULD] · 0 [MAY] — see inline threads for detail.

What this round resolved

The two [MUST] items from round 8 are closed:

• safeIdentifierRe duplication — fixed by exporting query.SafeIdentifierRe from internal/query/sqlutil.go and importing it in both bento.go and query.go. Single source of truth, compile-time link.
• registerOnce integration test correctness — the integration test now has a clear TestMain-wires-once contract documented in setup_test.go lines 10-13; the shared tiered cache flows from setup() through to StartIngestWorker and buildServer correctly.

Also well done: context.WithoutCancel for mutation execution, defer setCancel() inside singleflight closures (runs when closure returns, not when the outer request handler returns), OnEvict cleanup hook, l.ttls.Store now inside tagsMu.Lock(), and full test coverage for REPLACE INTO + column-prefix false-positive invariant.

Remaining findings

Thread	File	Severity
[SHOULD] Expiry TOCTOU in Get: author's prior reply said "verify again the TTL status" after acquiring the lock, but the code at lines 114–120 has no re-check — a concurrent Set can write a fresh entry that the expiry eviction then silently deletes	internal/cache/local.go:120	Correctness: LOW
[SHOULD] registerOnce one-call-per-process constraint is undocumented in the StartIngestWorker godoc — callers that pass a second apiCache get no error	internal/ingest/bento.go:57	Correctness: LOW

Verdict: Ship it

No [MUST]-level findings remain. Both [SHOULD] items are low-probability correctness issues (spurious cache MISS and silent no-op argument) with no data-loss or security impact. The TOCTOU fix is a one-liner and the godoc addition is trivial — worth doing before merge, but not a blocker. Address both, resolve the threads, and this is ready.

[EricAndrechek]

We don't have any L2/shared cache right now... it's just the same as the general cache interface, so technically the interface does support this we just don't have an implementation right now but we can/should still use the interface as if we do, no?

[EricAndrechek]

See comments in file, but this needs revisions, should build it as if fully implemented/supported

[EricAndrechek]

Would like to talk about this file and help understanding it in-person review later, lot's of questions/confusion on it and could use a walkthrough

[EricAndrechek]

Why? Why not leave as TieredCache? They do the same things for us under the hood, no?

[EricAndrechek]

Should this not be the same context as the setCtx is here for the cache set, why/why not?

[EricAndrechek]

I really don't like all the regex in this file... it scares me... also worries me that is may go out of sync with regex we have in the internal query package. How much of this is needed here vs should/could be shared and may be useful more generally, and/or what of this do you think will be removed anyway when we work on #167?

[EricAndrechek]

Same note on this file, why not TieredCache?

[EricAndrechek]

The more I look at this/see it in several places, the more it concerns me – we don't know that 5sec will be enough and is a clear path to DoS risks and feels like a premature optimization that would be better tracked and implemented in one pass everywhere, as tracked by #120.

[EricAndrechek]

Right direction, but I think there are a significant number of things that need changing before approval. Namely:

• singleflight optimizations for dropped initial connections should be removed from this PR to keep it smaller, and tracked in #120 instead
• TieredCache should still be used and templated as if it works with an L2 cache, no harm if there is no layered cache but the interface for it should be the same regardless
• Not a huge fan of the regex, especially as part of the already bloated API package, let's consider cleaning it up/reorganizing where its put and if there are other/better ways we can do it, as this is effectively a custom partial lite SQL parser, and will become a thorn in our side with edge cases very quickly
• I need some more help making sense of the actual cache interface's implementation (in local.go), perhaps some more of it could be abstracted out/shared in a way to let it work with minimal repeated code regardless of if using ristretto or redis, etc.

[EricAndrechek]

Edit: Closing in favor of #164 first, it will make this issue significantly easier to address in a new PR.