fix(packages): migrate older DiskCache schemas to add leases.refcount#133
fix(packages): migrate older DiskCache schemas to add leases.refcount#133
Conversation
Older production cache databases (pre-#119) lack the `leases.refcount` column that `pin()`/`unpin()` expect, causing every call to `DiskCache::lease()` to error with "no such column: refcount" and blocking any `.lnk` pull against a long-lived cache. Replace the single-step `migrate_v1` with an append-only versioned migration framework tracked in a `schema_migrations` table, keyed by stable migration ids: - m001_initial_schema — original tables + indexes (idempotent CREATE TABLE IF NOT EXISTS). - m002_add_leases_refcount — ALTER TABLE adds refcount INTEGER NOT NULL DEFAULT 1 when the column is absent; no-op on new caches that already have it from m001. Each migration runs in its own transaction and is skipped if its id is already present in `schema_migrations`. Future deltas just append. The legacy `cache_meta.schema_version` key is still mirrored for tooling that inspects it, but the authoritative source of truth is now the migrations table. Revert the `best_effort_lease()` workaround in `lnk/resolver.rs` that #119 added: the lease is once again a hard requirement, since the migration guarantees the column exists on every opened cache. Tests: - `test_legacy_schema_missing_refcount_is_migrated` (unit): hand- seeds the pre-migration schema, opens via `CacheIndex::open`, asserts `pin()`/`unpin()` succeed. - `test_migrations_idempotent_across_reopens` (unit): second open must not re-apply migrations or double-error on ALTER TABLE. - `test_all_migrations_recorded_after_open` (unit): regression guard ensuring every registered migration is recorded. - `legacy_cache_migrates_and_lease_succeeds` (integration): full `DiskCache::open_at` → `cache.lease()` flow against a seeded legacy DB, directly reproducing the issue-#124 scenario. - `fresh_cache_lease_still_works` (integration): regression guard for the fresh-DB happy path. Closes #124 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Cuts a release containing the two P0 fixes landed since 2.1.19: - #134 "P0 regression — Operation not permitted (os error 1) on warm build" - #135 "preserve exec bit on fbuild console script in wheel" Both are currently blocking every FastLED uno build on GitHub Actions: the wheel's console script installs without +x, so CI can't even run `fbuild --version`, and the subsequent compile fails with `Operation not permitted (os error 1)` on every example. Also includes: - #131 rustfmt on lnk pipeline - #133 DiskCache leases.refcount schema migration - #128 AVR orchestrator fingerprint fast-path + telemetry (#127) - #126 FBUILD_WATCH_SET_CACHE_SECS env override - f8533d3 extend watch-set fingerprint fast-path to AVR orchestrator Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cuts a release containing the two P0 fixes landed since 2.1.19: - #134 "P0 regression — Operation not permitted (os error 1) on warm build" - #135 "preserve exec bit on fbuild console script in wheel" Both are currently blocking every FastLED uno build on GitHub Actions: the wheel's console script installs without +x, so CI can't even run `fbuild --version`, and the subsequent compile fails with `Operation not permitted (os error 1)` on every example. Also includes: - #131 rustfmt on lnk pipeline - #133 DiskCache leases.refcount schema migration - #128 AVR orchestrator fingerprint fast-path + telemetry (#127) - #126 FBUILD_WATCH_SET_CACHE_SECS env override - f8533d3 extend watch-set fingerprint fast-path to AVR orchestrator Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Fixes #124. Any
DiskCache::lease()call against a production cache DB created before theleases.refcountcolumn was added errors withno such column: refcount, blockingfbuild lnk pulland every other lease-consuming code path.Approach — option B (versioned migrations)
Replaces the single-step
migrate_v1with an append-only, idempotent migration framework keyed by stable ids in a newschema_migrationstable:m001_initial_schema— original tables + indexes, allCREATE TABLE IF NOT EXISTSso it's safe on databases that already have them.m002_add_leases_refcount—ALTER TABLE leases ADD COLUMN refcount INTEGER NOT NULL DEFAULT 1;guarded by aPRAGMA table_info(leases)check. No-op on caches that already have the column fromm001.Each migration runs inside its own transaction and is skipped if its id is already present in
schema_migrations. Future deltas are append-only. The legacycache_meta.schema_versionkey is still mirrored for tooling that inspects it, but the authoritative source of truth is now the migrations table.Chose option B over option A because the issue itself flags option 2 as the longer-term clean answer and the framework cost is small (~60 LOC) compared to re-adding a migration infrastructure the next time a column gets added.
best_effort_leaserevertThis PR also reverts the
best_effort_lease()workaround introduced in #119'slnk/resolver.rs. Since every opened cache now hasleases.refcountguaranteed by the migration, the lease is once again a hard requirement —cache.lease(entry).map_err(map_cache_err)?. Revert is in the same commit as the migration:90d18d0.Files touched
crates/fbuild-packages/src/disk_cache/index.rs— migration framework +MIGRATIONSlist +leases_has_refcounthelper + 3 new unit tests.crates/fbuild-packages/src/lnk/resolver.rs— removebest_effort_lease, usecache.lease()?directly.crates/fbuild-packages/tests/disk_cache_schema_migration.rs— new integration test seeding a legacy schema and assertingDiskCache::open_at+cache.lease()succeed end-to-end.crates/fbuild-packages/tests/README.md— new (required by repo's readme-guard hook).Test plan
uv run cargo test -p fbuild-packages— all 391 unit tests + 2 new migration integration tests + 4lnk_e2etests pass.uv run cargo clippy --workspace --all-targets -- -D warnings— clean.test_legacy_schema_missing_refcount_is_migratedreproduces the exact bug scenario (pre-migration DB → open → lease) before the fix and passes after.test_migrations_idempotent_across_reopensguards against re-runningALTER TABLEon second open.fresh_cache_lease_still_works) confirms no regression for new users.🤖 Generated with Claude Code