Document object-store hydration doctrine + plan idle-flush eviction - #901
Conversation
An object store is not a runtime store, and the reason is architectural rather than about speed: Lance opens a network-scheme URI natively, so the wrong topology *runs* and only degrades — while deleting the mmap layer underneath it. A remote read lands in a freshly allocated buffer, one copy per read, with no page cache, so every zero-copy guarantee below that point becomes a claim about bytes that were copied into existence. Splitting the three jobs (object store hydrates / local directory IS the store / the volume only decides whether hydration repeats) makes the correctness requirement visible: a local directory, of any kind. The volume is an optimization on hydration frequency and must not be stated as a dependency. The feature-gate half is written down because it cost a session an hour. `lancedb` ships `default = []` while `lance-io` carries `aws` in its own defaults, so "the Lance stack does object storage by default" is true one layer down and false at the layer we depend on — the diagnosis goes wrong because the mental model is correct about the wrong crate. The resulting error names a *scheme*, before any credential is read, so no amount of endpoint or region debugging can help. That gives a mechanical rule worth keeping: a scheme-named error is a build problem, a credential-named error is a config problem. `docs/DATAFUSION-PERIMETER.md` gets a cross-reference because that document already catalogues this exact shape one crate over. The plan builds on the same lifecycle but is deliberately not an implementation. It records the two things a future reader would otherwise re-derive or get wrong: that the purpose is cost smoothing rather than capacity (so the budget is soft and never fails an operation), and that a lease/refcount protocol was considered and rejected as disproportionate at a three-day idle floor — with the revisit condition named, so it is not re-added later on the assumption it was overlooked. The race therefore owes a "does not corrupt" test rather than an impossibility proof, since the latter would be exactly the code-implied assertion the falsifiability rule forbids. Acceptance criteria are fire/silence pairs; because the trigger is a conjunction, the silence half splits in two, and the sharper of them is what a staleness-only policy would fail. Docs only. No crate, type, feature or test changed. Board hygiene in the same commit per CLAUDE.md: EPIPHANIES x2 (prepend), INTEGRATION_PLANS x1 (prepend), LATEST_STATE (prepend). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a98628d3-5e68-4d14-919b-649ae5d8325f) |
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds documentation for S3-backed Lance hydration and feature gating. It also proposes an off-by-default idle-flush eviction policy with thresholds, race handling, observability, acceptance tests, and open implementation questions. No runtime code changes are included. ChangesHydration and idle eviction documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d950b4ae70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **If a read begins mid-flush, let it rehydrate.** Do not block it, do not | ||
| coordinate with the sweeper. The reader's own hydration path is already the | ||
| recovery mechanism — `absent → hydrated` is idempotent and safe to repeat. |
There was a problem hiding this comment.
When a read starts after the non-authoritative in-flight check but while the dataset directory is being deleted, an uncoordinated hydration can race that deletion: the sweeper may remove newly hydrated files, or the reader may open a partially deleted/partially hydrated multi-file Lance dataset. The three-day threshold only lowers the race's frequency; it does not make absent → hydrated safe to overlap with eviction, so the stated worst case is not limited to a wasted rehydration. The plan needs an atomic publish/rename or per-dataset synchronization/retry protocol before rejecting coordination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — and this was the most valuable comment on the PR. You are right that the three-day floor lowers the race's frequency and does nothing for its safety, and right that the stated worst case did not follow.
The specific thing that was wrong: §5's "worst case is a wasted rehydration" rested entirely on absent → hydrated being idempotent. A Lance dataset is a multi-file directory, so a repeatable transfer is not idempotence over the destination. Three interleavings the plan left open, now enumerated in §5a: a reader resolving the path while files disappear; a hydration and a reclaim overlapping so the sweeper removes what was just written; a hydration failing mid-transfer and its debris later read as a present dataset. Two are torn reads, one is a silent wrong answer.
The fix, and why it is not the protocol the operator rejected — new plan §5a and knowledge doc §4a:
Hydrate aside; publish by rename; retire by rename. Fetch into a private temporary directory; make it visible with one atomic directory rename. Reclaim by renaming away first, deleting the renamed copy afterwards. A reader resolves the published name once and holds what it resolved.
This is a filesystem-atomicity boundary, not a coordination protocol. It costs the sweeper a rename and the reader nothing — no guard in a read signature, no per-read atomic state step, no refcount, no lease, no second gate-off code path. Those were exactly the costs that priced out the lease protocol in §5, so the operator's scope ruling stands and the corruption hole closes. Failure debris is self-identifying (never published), which closes the third case without a partial-state protocol, and the rename-away is the hydrated → flushed barrier.
Acceptance criteria updated: T9 now forces all three interleavings rather than one convenient one, and T9b asserts the boundary directly so T9 cannot pass on timing luck. The assumption it rests on — directory rename is atomic on the filesystem in use — is stated, and falls inside the "supported, mmap-capable local filesystem" requirement added elsewhere in this round.
Commit ea33f3b.
Generated by Claude Code
| **`lancedb` ships `default = []`.** Its `aws` feature is what forwards to | ||
| `lance/aws` + `lance-io/aws` (+ `object_store/aws`), and *that* forwarding is | ||
| what registers the S3 provider. |
There was a problem hiding this comment.
For the repository's existing S3 path, VersionedGraph::s3 ultimately calls lance::dataset::Dataset::open, and crates/lance-graph/Cargo.toml depends directly on lance = "=9.0.0"; the lancedb dependency is optional and no production source uses it to open these datasets. Consequently, a provider error from VersionedGraph::s3 cannot be diagnosed or fixed from lancedb's default-feature setting as this section instructs. Scope this advice to callers that actually use lancedb, and document the direct lance feature path for this crate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted, verified, and it is worse than you put it — the document fell into the trap it exists to teach.
Checked directly:
crates/lance-graph/Cargo.toml:38—lance = "=9.0.0", direct, non-optional, default features.lance's own manifest —default = ["aws", "azure", "gcp", ...]. So the provider is compiled in here.crates/lance-graph/Cargo.toml:41—lancedb = { version = "=0.33.0", optional = true, default-features = false }, behind thelancedb-sdkfeature.VersionedGraph::{s3,azure,gcs}(graph/versioned.rs:113-137) store the URI and the read path opens it — throughlance, exactly as you said.
So §3 was correct about lancedb and stated as though it were the gate on this repo's reads. That is the §3 failure mode itself — "the mental model is correct about the wrong crate" — one layer further out, in the document that names it.
Fixed as §3a, with the rule's missing first step made explicit: resolve which crate opens the URI, then read that crate's features — and check how this manifest takes it, since a default-features = false on the dependency line overrides the upstream default. A per-consumer table distinguishes the lance case (feature on) from the lancedb case (feature off). Same correction applied to docs/DATAFUSION-PERIMETER.md §9a, which carried the same claim.
The evidence table gained a row for this with the probe commands, their output and an explicit promotion decision: the manifest rows stay FINDING, the inference the first draft drew from them is corrected, not promoted. Commit ea33f3b.
Generated by Claude Code
| > Over a measurement window, compute | ||
| > **`rehydrations / distinct_datasets_accessed`**. | ||
| > A value **> 1.0** means at least one dataset was evicted and re-fetched | ||
| > *within its own working set* — the definition of thrash. |
There was a problem hiding this comment.
If the measurement window is longer than the three-day age floor, a dataset can be accessed, remain idle past the floor, be correctly evicted, and later begin a new working-set interval; that produces two rehydrations for one distinct dataset and a ratio above 1.0 without thrashing. Since this ratio is designated the gating acceptance criterion, normal sparse usage would raise false alarms. Bound the window to the age floor or count distinct dataset working-set intervals rather than distinct dataset IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — the false-alarm case you describe is exactly the policy working correctly, which makes it the worst possible thing for a gating criterion to flag.
Your scenario: accessed → idle past the floor → correctly evicted → new working-set interval later. Two rehydrations, one dataset, ratio above 1.0, no thrash. As a gate, that would have made normal sparse usage — the population this feature targets — read as failure.
You offered two fixes and the corrected §7 takes both, because they close different halves:
- Bound the window to the age floor. Inside one floor-length window a correctly-evicted dataset cannot legitimately be re-accessed; re-access within the floor is precisely what "the floor was too short for this dataset" means. Your scenario now spans two windows and registers as zero.
- Count intervals, not raw hydrations. Implemented as an eviction stamp: the sweeper marks each dataset it reclaims, and only a hydration that finds a stamp increments
eviction_caused_rehydrations. First hydrations are excluded by construction, and so are restarts, retries, invalidation and version reloads.
One further change your comment led to: with the numerator correctly restricted, > 1.0 could no longer fire when one dataset among many thrashes. The threshold is now > 0 on the corrected numerator, with the ratio retained as a severity measure rather than the trigger.
T11 rewritten so its silence half is your exact scenario; T11b added to assert the attribution itself. Commit ea33f3b.
Generated by Claude Code
| - **Never open a network-scheme URI as the runtime store.** Hydrate to a local | ||
| path, open the local path. If a design opens the remote URI directly, the | ||
| finding is not "this is slow" — it is that the zero-copy layer has been | ||
| removed. |
There was a problem hiding this comment.
This categorical rule conflicts with the existing public VersionedGraph::s3, azure, and gcs constructors: their read methods pass the resulting network URI directly to Dataset::open, and tests explicitly preserve those remote paths. Because this documentation-only commit neither deprecates those constructors nor adds the proposed hydration layer, it declares supported public flows architecturally invalid without giving callers a usable replacement. Scope the rule to the hot zero-copy substrate or include a migration plan for those APIs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted — verified, and the criticism is fair.
crates/lance-graph/src/graph/versioned.rs:113-137 ships VersionedGraph::{s3, azure, gcs}; each stores the URI as base_path and the read path passes it straight through, and the tests at :778 and :784 do preserve those remote paths. §6's first bullet, written categorically, therefore condemned a shipped public API while offering no replacement — which produces silent non-compliance, not migration.
Scoped as §6a:
- What the rule binds: the hot zero-copy substrate — any read path whose correctness story includes mapped bytes, a lens over them, or a page-cache assumption. There a network scheme voids the guarantee rather than degrading it, which is the structural finding.
- What it does not bind: occasional non-hot access making no zero-copy claim — administrative reads, one-off inspection, a version listing. The remote constructors remain the right tool there, and calling one is not a violation.
- Where that leaves them: usable and unmigrated, which is now a known state. The instruction is choose by read shape, not by constructor availability.
You also asked for a migration plan, and I have deliberately not invented one. The missing piece is a hydrating counterpart (hydrate_from(remote) → local, published via the rename boundary added in §5a this round). It is recorded as ISS-REMOTE-URI-CONSTRUCTORS-PREDATE-THE-HYDRATION-DOCTRINE rather than sketched, because it is new public API on a shipped type, this PR is documentation-only, and the plan it would share machinery with still carries an unclosed verification gate (§4). Shipping that surface before the gate closes would shape it around an unchecked assumption.
Nothing is deprecated. Commit ea33f3b.
Generated by Claude Code
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
.claude/plans/idle-flush-dataset-eviction-v1.md (1)
239-243: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an acceptance test for incompatible feature gates.
Lines 239-243 require
idle-flush=onwith the object-store provider disabled to fail at build time or startup. T1-T12 do not cover this combination. Add an acceptance criterion that checks the selected failure phase and prevents the first eviction from reaching the scheme lookup path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/plans/idle-flush-dataset-eviction-v1.md around lines 239 - 243, Add an acceptance criterion to the idle-flush feature-gate requirements covering idle-flush enabled with the object-store provider disabled. Require the configuration to fail during build or startup, and verify no first-eviction runtime path reaches scheme lookup; update the T1-T12 coverage or add a dedicated acceptance test for this combination.docs/DATAFUSION-PERIMETER.md (1)
334-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd source links for the object-store error rule.
Lines 35-41 are source-verified manifest facts. Lines 43-48 publish object_store runtime behavior from inference only. Add the pinned
object_store,lance-io, andlancedbversions and the exact source path/test that shows scheme parsing happens before credential/endpoint resolution, or label this as an inference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DATAFUSION-PERIMETER.md` around lines 334 - 347, Update the object-store error rule in the documented section to either add pinned object_store, lance-io, and lancedb versions with exact source paths and tests demonstrating scheme parsing precedes credential, endpoint, and region resolution, or explicitly label the rule as inference. Keep the existing manifest facts and diagnostic distinction intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/board/EPIPHANIES.md:
- Line 13: Clarify the lifecycle documentation by separating local eviction from
object-store push-back: define distinct operations and states for clean local
eviction versus persisting dirty data. Update the lifecycle and flush rule so
eviction cannot delete dirty data, while dirty objects must be persisted before
removal; preserve the asserted prohibition on a direct dirty → flushed
transition.
- Line 21: Expand the cost-smoothing rationale in the eviction policy section to
account for object-store request, retrieval, data-transfer, and
storage-management charges alongside retained-byte storage. State the deployment
context and storage-class assumptions before using the three-day and 300 MB
defaults to justify eviction, while preserving the requirement that eviction
never causes an operation to fail.
- Line 31: The thrash acceptance criterion must count only rehydrations
attributable to eviction, not all hydration retries or reloads. Update the
instrumentation described in Q5 to record an eviction cause or generation
identifier, define the measurement window, and compute rehydrations /
distinct_datasets_accessed from eviction-caused rehydrations while preserving
the existing second-hydration and amortization metrics.
- Line 29: Update the eviction notes around the Q4 race-guard decision to record
the forced multi-process race interleaving, including atomic directory
visibility, mmap lifetime, and cross-process coordination requirements. Keep the
eviction gate disabled unless multi-process access is explicitly out of scope or
supported and tested, and document that boundary alongside the existing
rejection rationale.
In @.claude/knowledge/s3-hydration-lifecycle.md:
- Around line 120-128: Update the “absent → hydrated” transition documentation
to make repeatability conditional on a fixed source version and an empty,
quiescent destination. Document hydration into a temporary directory followed by
atomic publication, or specify an equivalent recovery rule for partial, stale,
or mixed-version directories; remove the unconditional “safe to repeat” claim.
- Around line 23-35: Update the evidence table and related architectural
sections to record a complete claim → probe → run → promotion chain for every
architectural claim: include the exact probe command, observed result, and
promotion decision. Until those records exist, relabel claims supported only by
local inspection or another session as CONJECTURE or PROBE RESULT rather than
FINDING, and ensure the lifecycle guidance does not present unverified claims as
unconditional rules.
- Around line 146-180: Revise the performance conclusions in the
measured-characteristics section to scope them to the stated endpoint, workload,
and approximately 35 MB/tens-of-thousands-of-rows dataset. Remove or qualify
unsupported generalizations about RAM/NVMe ratios and large-dataset boot
viability, explicitly noting that transfer time scales with dataset size (for
example, 1 GiB at ~21 MiB/s takes roughly 49 seconds before overhead). Retain
the measured-case hydration conclusion while making clear it does not establish
viability for larger datasets.
- Around line 39-47: Update the “local directory” requirement in the lifecycle
table and the corresponding wording around lines 71–75 to require a supported,
mmap-capable filesystem with appropriate locking and consistency semantics,
rather than accepting any local path. Keep persistent-volume availability
separate as a hydration-frequency optimization, not as the filesystem
correctness requirement.
- Around line 49-52: Update the “Why the object store must not be the store”
section to qualify Lance’s native s3:// opening behavior on the lancedb aws
feature being enabled. Keep the diagnostic guidance specific to failures in the
S3 provider lookup path, rather than presenting scheme errors as universal.
- Around line 130-132: Update the flush contract in the lifecycle plan to
require an atomic guard for the hydrated-to-flushed transition before local
deletion, such as an atomic barrier, eviction lease, or locked recheck.
Explicitly prevent writer-induced dirty-to-flushed races and replace the current
non-corruption claim that relies only on the 3-day floor.
In @.claude/plans/idle-flush-dataset-eviction-v1.md:
- Around line 252-263: Clarify the thrash metric by defining rehydrations as an
explicit eviction-triggered rehydration count, excluding initial hydration and
datasets already resident at window start. Update T11 to use this counter and
make a single eviction/refetch count as thrash, rather than relying on
rehydrations / distinct_datasets_accessed > 1.0.
- Around line 319-323: Resolve the dirty-candidate policy consistently across
all three records: choose whether the sweep skips dirty candidates or initiates
push-back, define the chosen behavior and corresponding acceptance criteria in
.claude/plans/idle-flush-dataset-eviction-v1.md (lines 319-323), update the
push-back statement in .claude/board/INTEGRATION_PLANS.md (lines 3-6) to match,
and record the same behavior and status in .claude/board/LATEST_STATE.md (lines
5-6).
- Around line 198-212: Update the design around the flush/rehydration boundary
to require an atomic local-store swap, such as preparing a complete generation
directory and replacing the active directory only after it is fully populated.
Update T9 to exercise a read interleaving during the swap and assert that
readers observe either the old or new complete dataset, never a partially
removed or incomplete one.
---
Nitpick comments:
In @.claude/plans/idle-flush-dataset-eviction-v1.md:
- Around line 239-243: Add an acceptance criterion to the idle-flush
feature-gate requirements covering idle-flush enabled with the object-store
provider disabled. Require the configuration to fail during build or startup,
and verify no first-eviction runtime path reaches scheme lookup; update the
T1-T12 coverage or add a dedicated acceptance test for this combination.
In `@docs/DATAFUSION-PERIMETER.md`:
- Around line 334-347: Update the object-store error rule in the documented
section to either add pinned object_store, lance-io, and lancedb versions with
exact source paths and tests demonstrating scheme parsing precedes credential,
endpoint, and region resolution, or explicitly label the rule as inference. Keep
the existing manifest facts and diagnostic distinction intact.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e8db147-70b4-41c5-9498-efe75f2f2412
📒 Files selected for processing (6)
.claude/board/EPIPHANIES.md.claude/board/INTEGRATION_PLANS.md.claude/board/LATEST_STATE.md.claude/knowledge/s3-hydration-lifecycle.md.claude/plans/idle-flush-dataset-eviction-v1.mddocs/DATAFUSION-PERIMETER.md
…bearing
Every review comment evaluated against the code and the manifests before
acting; the corrections below are the ones that survived that check.
THE LOAD-BEARING ONE. Four comments converged on the same gap, and they were
right: the plan's §5 rejects a lease protocol and asserts a lost flush/read
race costs at worst a wasted rehydration — resting entirely on "absent →
hydrated is idempotent". A Lance dataset is a multi-file DIRECTORY, so a
repeatable TRANSFER is not idempotence over the DESTINATION. A partial fetch
leaves debris a retry merges into; a fetch against a different source version
mixes two snapshots; a concurrent reclaim can delete what a hydration just
wrote or expose a directory that is neither present nor absent. Two of those
are torn reads and one is a silent wrong answer. Rarity was being used where
recoverability was claimed.
Fix: hydrate aside, publish by rename, retire by rename (knowledge §4a, plan
§5a) — a filesystem-atomicity boundary, NOT the protocol the operator walked
back. It costs the sweeper a rename and the reader nothing: no guard in a
signature, no per-read atomic step, no refcount, no lease, no second gate-off
path. Every objection that priced out the lease protocol leaves it untouched,
so the operator's scope ruling stands and the hole closes. T9 sharpened to
three enumerated interleavings; T9b added for the boundary itself.
THE DOC FELL INTO ITS OWN TRAP. §3 teaches "diagnose the feature on the crate
that opens the URI", then diagnosed `lancedb`. Verified: this repo opens
datasets through `lance` — direct, non-optional, default features, and
`lance`'s own default includes `aws`; `lancedb` is optional,
`default-features = false`, behind its own flag, and no production path uses
it for these reads. Corrected as §3a with probe commands, output and an
explicit promotion decision. Same correction applied to
docs/DATAFUSION-PERIMETER.md §9a, which had it too.
A CATEGORICAL RULE CONDEMNED SHIPPED API. §6's "never open a network-scheme
URI" reads as invalidating the public, tested `VersionedGraph::{s3,azure,gcs}`
with no replacement offered. Scoped in §6a to the hot zero-copy substrate;
the missing hydrating counterpart recorded as
ISS-REMOTE-URI-CONSTRUCTORS-PREDATE-THE-HYDRATION-DOCTRINE rather than
implied. Not a deprecation — nothing is removed.
THREE OVER-CLAIMS. "Any local path" → a supported, mmap-capable local
filesystem (a network mount presents an ordinary directory while changing
page-cache, consistency and lock semantics); durability stays an
optimization, the filesystem is the correctness axis. "VIABLE for hydration"
is a verdict about access SHAPE, not size — ~1 GiB is ~49 s at the observed
rate, so boot-viability is a per-dataset question and only the tens-of-MB
case was measured; the RAM/NVMe ratios are conventional figures, not
measurements taken here. The cost model priced only retained bytes —
request/retrieval/transfer/storage-management named, storage-class assumption
stated, and the OPERATOR-SET grading of the two defaults explicitly protected
from drifting toward FINDING.
THE THRASH METRIC COULD NOT DO ITS JOB, three ways, all real: an unattributed
numerator (restarts, retries, invalidation, version reloads all counted), an
unbounded window that false-alarmed on correct sparse usage, and a `> 1.0`
threshold that could not fire when one dataset among many thrashes.
Redefined as `eviction_caused_rehydrations` (sweeper-stamped at reclaim, so
first hydrations are excluded by construction) over a window bounded to the
age floor, threshold `> 0`, ratio retained as severity. T11 rewritten, T11b
added to assert the attribution itself.
A CONTRADICTION BETWEEN THE PLAN AND ITS OWN SUMMARIES about dirty candidates
— resolved, not papered over: the sweep does CLEAN EVICTION ONLY (§9a) and
never initiates push-back. It follows from the one hard rule (flush is legal
only from `hydrated`): a sweep that pushed first would be manufacturing the
precondition for its own destructive step, unattended, on a timer. "Every
candidate dirty" becomes a third distinct stop reason; T6b asserts both
halves. The two board one-liners that said "pushed back first if dirty" are
corrected in place with the correction itself recorded — they are this PR's
own unmerged text, not merged history.
Board hygiene: EPIPHANIES PREPEND
(E-A-REPEATABLE-TRANSFER-IS-NOT-IDEMPOTENCE-OVER-A-MULTI-FILE-DIRECTORY-1 —
the two entries it corrects are left as written so the first draft's claims
stay visible), ISSUES PREPEND ×1, LATEST_STATE review-round bullet.
Still documentation-only: no crate, type, feature or test changed; nothing in
any Cargo.toml touched.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1f2dbe63-3efe-4991-b6b5-0b535628ed06) |
Review round complete — 17 comments, 16 fixed, 1 fixed-in-part, 0 ignoredEvery comment was evaluated against the code and the vendored manifests before acting, and each thread has a reply with the specific verification. Commit The one that mattered most. Four comments converged on the same gap and they were right: §5 rejected a lease protocol and asserted a lost flush/read race costs at worst a wasted rehydration — resting entirely on The fix — hydrate aside, publish by rename, retire by rename (knowledge §4a, plan §5a) — is a filesystem-atomicity boundary, not the coordination protocol the operator walked back. It costs the sweeper a rename and the reader nothing: no guard in a signature, no per-read atomic step, no refcount, no lease, no second gate-off path. Every objection that priced out the lease protocol leaves it untouched, so the scope ruling stands and the hole closes. Those turned out not to be in tension once the mechanism was in the right layer. The document fell into its own trap. §3 teaches "diagnose the feature on the crate that opens the URI" — and then diagnosed The rest, each with its own reply:
New acceptance criteria: T6b, T9b, T11b; T9 sharpened from one interleaving to three enumerated ones. One partial disagreement, in its thread: the request to add a run record behind every FINDING row was not taken wholesale. Rows graded Board hygiene: EPIPHANIES PREPEND ( Generated by Claude Code |
Documentation only. No crate, type, feature, or test changed;
Cargo.tomluntouched. CI here is Rust-only, so nothing in this PR is exercised by it.Part (a) — the hydration doctrine
NEW:
.claude/knowledge/s3-hydration-lifecycle.md(READ BY:header, per-claim evidence table, every row graded).Three layers, one job each:
The load-bearing points:
zero-copy-lens-law.mdone layer down.lancedbshipsdefault = []; itsawsfeature forwards tolance/aws+lance-io/aws(+object_store/aws), and that registers the provider. Meanwhilelance-iocarriesawsin its own defaults — so "the Lance stack does object storage by default" is true one layer down and false at the layer we depend on. Without it the URI fails at provider lookup by scheme, before any credential is read. Mechanical rule: a scheme-named error is a BUILD problem; a credential/host/region-named error is a CONFIG problem.hydrated, never fromdirty— that edge is data loss with no error.docs/DATAFUSION-PERIMETER.md§9a (new section) — cross-reference, because that document already catalogues this exact shape (capability behind a default-off feature, diagnosed at the wrong layer) one crate over.Part (b) — plan for a feature-gated idle-flush
NEW:
.claude/plans/idle-flush-dataset-eviction-v1.md— PROPOSAL. Nothing implemented, nothing measured.Board hygiene (same commit, per CLAUDE.md)
.claude/board/EPIPHANIES.mdE-OBJECT-STORE-HYDRATES-IT-DOES-NOT-STORE-1,E-IDLE-FLUSH-IS-COST-SMOOTHING-NOT-CAPACITY-AND-THE-3-DAY-FLOOR-PRICES-OUT-A-LEASE-PROTOCOL-1.claude/board/INTEGRATION_PLANS.md.claude/board/LATEST_STATE.mdAll board edits are strictly additive —
git diff --numstatshows 0 deletions on every file in this PR.Evidence labelling
Per the workspace rule, both documents carry a per-claim table. The manifest facts (
default = [], theawsforwarding,lance-io's own defaults) are source-verified in this session and labelled FINDING. The endpoint timings are reported measurement, not re-run here — one provider, one region, one point in time. The lifecycle and the whole plan are CONJECTURE / OPERATOR-SET with falsifiers stated inline. Nothing is promoted past its row.Generated by Claude Code
Summary by CodeRabbit