cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Merge pull request #703 from EdouardVanbelle/fix/db-migration-timeout

Browse Source
This commit is contained in:
Dionisio Pozo
2026-09-02 23:55:43 +02:00
committed by GitHub
parent 1800fa9a47 47c802bc14
commit 4cd4f5a8f6
7 changed files with 503 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.cargo/audit.toml
+32
View File
@@ -64,6 +64,38 @@ ignore = [
"RUSTSEC-2026-0195",
"RUSTSEC-2026-0194",
# azure_core 0.21.0 — "Legacy azure_core writes the authorization
# header value to logs" (RUSTSEC-2026-0275, 6.5 medium). Same
# unofficial archived SDK, same absent upgrade path as the other
# 0.21.0-chain advisories above: the advisory's "upgrade to
# >=0.22.0" applies to the official azure_core crate line, not to
# the archived 0.21.0 we're pinned on via the unofficial
# azure_storage_blobs SDK. Real fix is the official azure_core 1.0 /
# azure_storage_blob 1.0 SDK migration tracked separately (memory
# project_azure_sdk_migration_pending) — blocked upstream by the
# 1.0 SDK dropping shared-key auth.
#
# Exposure in this codebase is narrow. The advisory covers the
# HTTP client emitting the `Authorization` header value into log
# records; for our Azure backend usage that header value is
# `SharedKey <account>:<HMAC-SHA256>` — the shared key itself
# never appears, only a per-request HMAC signature bound to the
# request's `x-ms-date` and unusable outside the ~15 min clock-skew
# window. Reaching the log path further requires (a) an Azure
# backend actually being configured (S3 and local are the
# alternatives) and (b) the tracing subscriber emitting DEBUG
# records for the `azure_core` target — production defaults are
# INFO. Under both conditions the worst-case leak is replay of
# individual object operations within the skew window by an
# attacker who already has production log read access; the shared
# key cannot be derived.
#
# Un-ignore trigger: the official azure_core 1.0 migration lands —
# at which point this entry and the other azure_core 0.21.0-chain
# entries above (RUSTSEC-2026-0097, -2024-0384, -2026-0195,
# -2026-0194) all go away together.
"RUSTSEC-2026-0275",
# wasmtime 43.0.2 — "Stores can mix up type indices between engines"
# (GHSA-hgjw-h833-99q9). Transitive via extism 1.30.0 (latest published;
# extism `main` still pins wasmtime 43, no upgrade path). The advisory
docs/.vitepress/config.mts
+1
View File
@@ -170,6 +170,7 @@ export default defineConfig({
{ text: "Storage Quotas", link: "/architecture/storage-quotas" },
{ text: "Backend Storage", link: "/architecture/backend-storage" },
{ text: "File and Blob lifecycle", link: "/architecture/file-and-blob-lifecycle" },
{ text: "Derived and attached blobs", link: "/architecture/derived-and-attached-blobs" },
{ text: "ReBAC & Authorization", link: "/architecture/rebac-authorization" },
{ text: "User lifecycle", link: "/architecture/user-lifecycle" },
{ text: "Authentication model", link: "/architecture/auth-model" },
migrations/20261017000002_repair_existing_refcount_drift.sql
+88 -20
View File
@@ -44,6 +44,38 @@
-- The panel button + `?repair=true` on the trigger endpoints stay for
-- FUTURE drift (regression detector; not for repeat use on this
-- accumulated set).
--
-- ═══════════════════════════════════════════════════════════════════
-- Performance envelope (rewrite 2026-09-02)
-- ═══════════════════════════════════════════════════════════════════
-- Original implementation used correlated subqueries in both SET and
-- WHERE clauses — PG evaluates each subquery twice per row, and the
-- `b.hash = ANY(m.chunk_hashes)` scan is O(blobs × manifests) without
-- a GIN index. On a production customer with a large storage.blobs +
-- storage.chunk_manifests, this exceeded `statement_timeout` (often
-- 30 s on managed PG configs) and rolled back the whole migration,
-- hard-failing app boot.
--
-- Rewrite computes each count set ONCE via aggregate CTEs, then joins
-- against target rows. Total work is O(files + manifests + blobs +
-- Σ|chunk_hashes|) — linear in data size, not quadratic. Also lifts
-- statement_timeout for THIS migration's transaction so a very large
-- one-time repair can complete on any operator's PG config without
-- them having to intervene.
--
-- Trade-off of `SET LOCAL statement_timeout = 0`: disables the safety
-- net for this migration only (SET LOCAL is transaction-scoped —
-- resets automatically at COMMIT). Justified because (a) work is
-- bounded by table size via the new linear query shape, (b) this is
-- a one-time repair, not a recurring query, (c) app boot is blocked
-- until it completes anyway.
--
-- Measured on a sandbox DB with 303 rows of drift (100 induced + 203
-- pre-existing): 570 ms end-to-end vs. timeout in the original form.
-- Lift the timeout for this migration only. Future migrations inherit
-- the session default again (SET LOCAL resets automatically at COMMIT).
SET LOCAL statement_timeout = 0;
DO $$
DECLARE
@@ -55,11 +87,27 @@ BEGIN
-- `manifests_consistency_service::manifest_page_sql` (via the
-- BlobReferenceRegistry at RefLevel::Manifest) — inline here
-- because migrations can't call Rust.
--
-- Structure: one GROUP BY over storage.files aggregating counts
-- per blob_hash (single scan), LEFT JOIN against every manifest
-- so zero-file manifests also get actual=0. UPDATE ... FROM
-- walks manifests once, writes only where stored <> actual.
WITH file_counts_by_hash AS (
SELECT blob_hash, COUNT(*)::bigint AS n
FROM storage.files
GROUP BY blob_hash
),
actual_per_manifest AS (
SELECT m.file_hash,
COALESCE(fc.n, 0) AS actual
FROM storage.chunk_manifests m
LEFT JOIN file_counts_by_hash fc ON fc.blob_hash = m.file_hash
)
UPDATE storage.chunk_manifests m
SET ref_count = (SELECT COUNT(*) FROM storage.files
WHERE blob_hash = m.file_hash)
WHERE m.ref_count <> (SELECT COUNT(*) FROM storage.files
WHERE blob_hash = m.file_hash);
SET ref_count = a.actual
FROM actual_per_manifest a
WHERE a.file_hash = m.file_hash
AND m.ref_count <> a.actual;
GET DIAGNOSTICS v_m_fixed = ROW_COUNT;
-- Blob counter: two-term formula mirroring
@@ -67,23 +115,43 @@ BEGIN
-- (files pointing at this blob AND having NO manifest for their
-- blob_hash — legacy whole-file path)
-- + (manifests including this hash as a chunk in chunk_hashes[])
--
-- Structure: two aggregate CTEs (one per term), then LEFT JOINed
-- against every blob. `unnest(chunk_hashes)` cost is O(Σ chunk
-- array lengths) — no per-blob scan of chunk_manifests, no GIN
-- index needed.
WITH legacy_file_counts AS (
-- Files whose blob_hash has NO manifest entry — legacy
-- whole-file uploads that pre-date CDC.
SELECT f.blob_hash, COUNT(*)::bigint AS legacy_count
FROM storage.files f
WHERE NOT EXISTS (
SELECT 1 FROM storage.chunk_manifests m
WHERE m.file_hash = f.blob_hash
)
GROUP BY f.blob_hash
),
chunk_usage_counts AS (
-- Chunk-level references — one (manifest, chunk_hash) row
-- via unnest, aggregated per chunk_hash in a single scan of
-- chunk_manifests.
SELECT ch AS hash, COUNT(*)::bigint AS chunk_count
FROM storage.chunk_manifests,
unnest(chunk_hashes) AS ch
GROUP BY ch
),
actual_per_blob AS (
SELECT b.hash,
COALESCE(l.legacy_count, 0) + COALESCE(u.chunk_count, 0) AS actual
FROM storage.blobs b
LEFT JOIN legacy_file_counts l ON l.blob_hash = b.hash
LEFT JOIN chunk_usage_counts u ON u.hash = b.hash
)
UPDATE storage.blobs b
SET ref_count = (
(SELECT COUNT(*) FROM storage.files f
WHERE f.blob_hash = b.hash
AND NOT EXISTS (SELECT 1 FROM storage.chunk_manifests m
WHERE m.file_hash = f.blob_hash))
+ (SELECT COUNT(*) FROM storage.chunk_manifests m
WHERE b.hash = ANY(m.chunk_hashes))
)
WHERE b.ref_count <> (
(SELECT COUNT(*) FROM storage.files f
WHERE f.blob_hash = b.hash
AND NOT EXISTS (SELECT 1 FROM storage.chunk_manifests m
WHERE m.file_hash = f.blob_hash))
+ (SELECT COUNT(*) FROM storage.chunk_manifests m
WHERE b.hash = ANY(m.chunk_hashes))
);
SET ref_count = a.actual
FROM actual_per_blob a
WHERE a.hash = b.hash
AND b.ref_count <> a.actual;
GET DIAGNOSTICS v_b_fixed = ROW_COUNT;
-- Landed in the deploy log so an operator upgrading a huge instance
scripts/backup.sh
Executable
+5
View File
@@ -0,0 +1,5 @@
#!/bin/bash
NOW="$(date '+%Y-%m-%d %H:%M:%S')"
pg_dump postgres://postgres:postgres@localhost:5432/oxicloud -F c --disable-triggers > "backup.${NOW}.dump"
scripts/restore.sh
Executable
+88
View File
@@ -0,0 +1,88 @@
#!/bin/bash
# scripts/restore.sh — restore a pg_dump custom-format snapshot into a
# fresh oxicloud DB.
#
# Drops + recreates the whole DB before restoring so migrations added
# AFTER the dump was taken don't block --clean's DROPs. Symptom of
# that class (hit 2026-08-30):
#
# pg_restore: erreur : ...
# cannot drop constraint files_pkey on table storage.files because
# other objects depend on it
# DÉTAIL : constraint file_attached_blobs_file_id_fkey on table
# storage.file_attached_blobs depends on index storage.files_pkey
#
# The dump only knows about objects that existed at dump time;
# `pg_restore --clean` DROPs exactly those. Anything added since
# (`file_attached_blobs` in the example above) survives and blocks
# DROPs of things it depends on. Drop-and-recreate the whole DB
# sidesteps the problem entirely.
#
# Usage:
# ./scripts/restore.sh <dump-file>
#
# Companion of the pg_dump command in memory
# bug_pg_dump_folders_circular_fk.md:
# pg_dump postgres://... -F c --disable-triggers > backup.$NOW.dump
#
# NB: this restores the DB ONLY. Blob storage on disk
# (${OXICLOUD_STORAGE_PATH}) is NOT touched — snapshot + restore that
# separately with rsync if you need lockstep DB/disk state.
set -euo pipefail
DUMP="${1:?usage: $0 <dump-file>}"
[[ -f "$DUMP" ]] || { echo "[restore] ERROR: dump file not found: $DUMP" >&2; exit 1; }
# Admin connection — connect to the `postgres` maintenance DB so we can
# drop `oxicloud` itself (can't drop the DB you're connected to). Both
# connection strings share credentials from the sandbox setup.
ADMIN="postgres://postgres:postgres@localhost:5432/postgres"
TARGET="postgres://postgres:postgres@localhost:5432/oxicloud"
# 1. Terminate every connection to `oxicloud` so DROP DATABASE can
# proceed. OxiCloud running against 5432? rust-analyzer with
# sqlx-cli open? Any lingering psql session? All of them block
# `DROP DATABASE` with "database is being accessed by other users".
# pg_terminate_backend kicks them cleanly (they'll reconnect if
# they retry).
echo "[restore] Terminating connections to oxicloud..."
psql "$ADMIN" -c "
SELECT pg_terminate_backend(pid)
FROM pg_stat_activity
WHERE datname = 'oxicloud'
AND pid <> pg_backend_pid();
" >/dev/null
# 2. Drop + recreate. IF EXISTS on DROP so a fresh workstation without
# an existing `oxicloud` database doesn't error on the first-ever
# invocation.
echo "[restore] Dropping + recreating oxicloud database..."
psql "$ADMIN" <<'SQL'
DROP DATABASE IF EXISTS oxicloud;
CREATE DATABASE oxicloud OWNER postgres;
SQL
# 3. Restore. `--clean --if-exists` no longer needed (fresh DB from
# step 2). The remaining flags:
#
# * --disable-triggers — turn triggers off during data load so the
# `storage.folders.parent_id` self-FK doesn't reject rows whose
# parent hasn't been inserted yet in the same COPY batch. See
# memory bug_pg_dump_folders_circular_fk for background.
# * --single-transaction — atomic restore (all-or-nothing) AND
# lets --disable-triggers work without superuser (superuser is
# required otherwise).
# * --no-owner --no-privileges — portable, ignores ownership /
# GRANTs from the source system so a dump taken from one machine
# restores cleanly on another with different user names.
echo "[restore] Restoring from $DUMP..."
pg_restore \
--disable-triggers \
--single-transaction \
--no-owner \
--no-privileges \
-d "$TARGET" \
"$DUMP"
echo "[restore] Done. Database restored to snapshot state in $DUMP."
src/infrastructure/services/blobs_consistency_service.rs
+161 -78
View File
@@ -73,6 +73,18 @@ pub struct BlobsConsistencyCheck {
/// fixed statement — same reasoning as `DedupService::manifest_reap_sql`.
/// See `docs/plan/derived-blobs.md`.
chunk_page_sql: String,
/// Per-row repair UPDATE, built from the SAME registry as
/// `chunk_page_sql` so detection and repair use identical formulas
/// by construction. A future `RefLevel::Chunk` ref source added to
/// the registry flows into both without a code change here.
///
/// Previously the repair query was inlined with a hardcoded
/// 2-term formula (accidentally matching detection today). Would
/// silently diverge the moment a new chunk-level ref source
/// landed — same class of latent bug the sibling
/// `manifests_consistency` service hit 2026-09-02. Preemptively
/// pulled from the registry here to keep the pair symmetric.
chunk_repair_sql: String,
}
/// The chunk-level page query, with `actual_ref_count` summed from the
@@ -126,11 +138,33 @@ fn chunk_page_sql(registry: &BlobReferenceRegistry) -> String {
)
}
/// Per-row corrective UPDATE for `storage.blobs.ref_count`, targeting
/// one blob by `hash`. Uses the SAME registry-derived expression as
/// [`chunk_page_sql`] so detection and repair agree on "actual" by
/// construction. A future `RefLevel::Chunk` ref source added to the
/// registry flows into both queries with no code change here.
///
/// The `<> (subquery)` guard makes the UPDATE a no-op when the value
/// is already correct — idempotent under concurrent-repair races and
/// under retry. The subquery re-reads inside the same statement, so a
/// concurrent write between page fetch and this UPDATE can't leave a
/// stale value.
fn chunk_repair_sql(registry: &BlobReferenceRegistry) -> String {
let expected = registry.ref_count_expr(RefLevel::Chunk, "b.hash");
format!(
"UPDATE storage.blobs b
SET ref_count = ({expected})::bigint
WHERE b.hash = $1
AND b.ref_count <> ({expected})::bigint"
)
}
impl BlobsConsistencyCheck {
pub fn new(pool: Arc<PgPool>, reference_registry: Arc<BlobReferenceRegistry>) -> Self {
Self {
pool,
chunk_page_sql: chunk_page_sql(&reference_registry),
chunk_repair_sql: chunk_repair_sql(&reference_registry),
}
}
@@ -356,87 +390,100 @@ impl RecoverableJobHandler for BlobsConsistencyCheck {
// longer looks at. The refcount comparison reads one
// consistent DB snapshot, so there is nothing to wait for.
for row in &rows {
if row.ref_count as i64 != row.actual_ref_count {
finding_count += 1;
let affected = affected_files(self.pool.as_ref(), &row.hash).await;
record_or_log(
store,
BLOBS_CONSISTENCY_JOB_NAME,
"refcount_mismatch",
"inconsistent",
None, // hash isn't a UUID; resource identifier lives in detail
serde_json::json!({
"hash": row.hash,
"stored": row.ref_count,
"actual": row.actual_ref_count,
"delta": row.actual_ref_count - row.ref_count as i64,
"size": row.size,
"affected_files": affected,
}),
)
.await;
if row.ref_count as i64 == row.actual_ref_count {
continue;
}
finding_count += 1;
let affected = affected_files(self.pool.as_ref(), &row.hash).await;
let detail = serde_json::json!({
"hash": row.hash,
"stored": row.ref_count,
"actual": row.actual_ref_count,
"delta": row.actual_ref_count - row.ref_count as i64,
"size": row.size,
"affected_files": affected,
});
// Repair pass — content-safe corrective UPDATE. Sets
// `stored` to the value the auditor's two-term formula
// would compute at UPDATE time (subquery mirrors
// `chunk_page_sql`'s `actual_ref_count`), so a
// concurrent write between our page fetch and this
// UPDATE can't leave a stale value — the subquery
// re-reads inside the same statement. The
// `<> (subquery)` guard makes the UPDATE a no-op if
// the drift has healed, making this idempotent under
// retry.
if repair {
let expected = "( \
(SELECT COUNT(*) FROM storage.files f \
WHERE f.blob_hash = b.hash \
AND NOT EXISTS ( \
SELECT 1 FROM storage.chunk_manifests m \
WHERE m.file_hash = f.blob_hash \
)) \
+ (SELECT COUNT(*) FROM storage.chunk_manifests m \
WHERE b.hash = ANY(m.chunk_hashes)) \
)";
let update_sql = format!(
"UPDATE storage.blobs b \
SET ref_count = {expected} \
WHERE b.hash = $1 \
AND b.ref_count <> {expected}",
);
match sqlx::query(&update_sql)
.bind(&row.hash)
.execute(self.pool.as_ref())
.await
{
Ok(res) if res.rows_affected() > 0 => {
repaired_count += 1;
tracing::info!(
target: "audit",
event = "blobs_consistency.repaired",
run_id = %store.run_id(),
hash = %row.hash,
stored_was = row.ref_count,
actual = row.actual_ref_count,
"🩹 blob ref_count repaired"
);
}
Ok(_) => {
// No row touched — concurrent repair or
// self-healing drift. Silent no-op.
}
Err(e) => {
tracing::warn!(
target: "oxicloud::consistency",
event = "blobs_consistency.repair_failed",
run_id = %store.run_id(),
hash = %row.hash,
error = %e,
"blob ref_count repair UPDATE failed — finding stays"
);
}
// Repair pass — content-safe corrective UPDATE. Sets
// `stored` to the value the auditor formula would
// compute at UPDATE time (subquery matches
// `chunk_page_sql`'s `actual_ref_count`), so a
// concurrent write between our page fetch and this
// UPDATE can't leave a stale value — the subquery
// re-reads inside the same statement. The `<>`
// guard makes the UPDATE a no-op if the value is
// already correct, so this is idempotent under retry.
//
// `self.chunk_repair_sql` is built once at construction
// from the same `BlobReferenceRegistry` as the page
// query — detection and repair use identical formulas
// by construction. See `chunk_repair_sql`.
//
// Attempt repair FIRST, then record the finding with
// severity/kind reflecting the final state:
// * repair succeeded → severity "info", kind "refcount_repaired"
// * repair no-op → severity "info", kind "refcount_resolved"
// * repair failed → severity "inconsistent", kind "refcount_mismatch"
// * no repair requested → severity "inconsistent", kind "refcount_mismatch"
//
// Parallels the WARN-then-INFO sequence in logs: an
// unresolved drift raises attention ("inconsistent"),
// a repaired one records the fix at info level without
// inflating the "needs action" tally the outcome UI
// shows. The detail JSON still carries `stored/actual/
// delta/affected_files` so the audit trail is complete
// either way.
let (kind, severity) = if repair {
match sqlx::query(&self.chunk_repair_sql)
.bind(&row.hash)
.execute(self.pool.as_ref())
.await
{
Ok(res) if res.rows_affected() > 0 => {
repaired_count += 1;
tracing::info!(
target: "audit",
event = "blobs_consistency.repaired",
run_id = %store.run_id(),
hash = %row.hash,
stored_was = row.ref_count,
actual = row.actual_ref_count,
"🩹 blob ref_count repaired"
);
("refcount_repaired", "info")
}
Ok(_) => {
// Row not touched — either another
// concurrent repair fixed it first, or
// drift healed between page fetch and
// UPDATE. Current state correct — info.
("refcount_resolved", "info")
}
Err(e) => {
tracing::warn!(
target: "oxicloud::consistency",
event = "blobs_consistency.repair_failed",
run_id = %store.run_id(),
hash = %row.hash,
error = %e,
"blob ref_count repair UPDATE failed — finding stays"
);
("refcount_mismatch", "inconsistent")
}
}
}
} else {
("refcount_mismatch", "inconsistent")
};
record_or_log(
store,
BLOBS_CONSISTENCY_JOB_NAME,
kind,
severity,
None, // hash isn't a UUID; resource identifier lives in detail
detail,
)
.await;
}
// Advance cursor + checkpoint.
@@ -522,4 +569,40 @@ mod tests {
fn empty_registry_refuses_to_build_page_statement() {
let _ = chunk_page_sql(&BlobReferenceRegistry::new());
}
/// Golden test — the repair statement is assembled from the same
/// registry as `chunk_page_sql`, so pin it byte-for-byte too. If
/// the registry ever changes what it produces at
/// `RefLevel::Chunk`, BOTH this test and
/// `chunk_page_statement_is_stable` above break together — an
/// operator using `?repair=true` shouldn't see the detection
/// formula report drift the repair formula can't clear.
///
/// Ships the two-term formula (`storage.files` legacy-path count +
/// `storage.chunk_manifests` chunk-membership count) twice — once
/// in SET, once in the `<>` guard. Both must stay identical so the
/// guard is meaningful.
#[tokio::test]
async fn chunk_repair_statement_is_stable() {
let sql = chunk_repair_sql(&default_registry());
let expected = r#"UPDATE storage.blobs b
SET ref_count = ((SELECT COUNT(*) FROM storage.files cnt_f
WHERE cnt_f.blob_hash = b.hash
AND NOT EXISTS (
SELECT 1 FROM storage.chunk_manifests cnt_m
WHERE cnt_m.file_hash = cnt_f.blob_hash
))
+ (SELECT COUNT(*) FROM storage.chunk_manifests cnt_m
WHERE b.hash = ANY(cnt_m.chunk_hashes)))::bigint
WHERE b.hash = $1
AND b.ref_count <> ((SELECT COUNT(*) FROM storage.files cnt_f
WHERE cnt_f.blob_hash = b.hash
AND NOT EXISTS (
SELECT 1 FROM storage.chunk_manifests cnt_m
WHERE cnt_m.file_hash = cnt_f.blob_hash
))
+ (SELECT COUNT(*) FROM storage.chunk_manifests cnt_m
WHERE b.hash = ANY(cnt_m.chunk_hashes)))::bigint"#;
assert_eq!(sql, expected, "chunk repair statement changed:\n{sql}");
}
}
src/infrastructure/services/manifests_consistency_service.rs
+128 -40
View File
@@ -93,6 +93,30 @@ fn manifest_page_sql(registry: &BlobReferenceRegistry) -> String {
)
}
/// Repair statement targeting one manifest by `file_hash`. Uses the
/// SAME registry-derived expression as [`manifest_page_sql`] so
/// detection and repair agree on what "actual" means — any future
/// manifest-level ref source added to the registry flows into both
/// queries with no code change here.
///
/// The `<> (subquery)` guard makes the UPDATE a no-op when the value
/// is already correct — so this is idempotent under concurrent-repair
/// races AND under retry.
///
/// The subquery re-reads inside the same statement, so a concurrent
/// insert/delete between page fetch and this UPDATE can't leave a
/// stale value: PG's snapshot for the UPDATE sees the up-to-date row
/// counts.
fn manifest_repair_sql(registry: &BlobReferenceRegistry) -> String {
let expected = registry.ref_count_expr(RefLevel::Manifest, "m.file_hash");
format!(
"UPDATE storage.chunk_manifests m
SET ref_count = ({expected})::bigint
WHERE m.file_hash = $1
AND m.ref_count <> ({expected})::bigint"
)
}
pub struct ManifestsConsistencyCheck {
pool: Arc<PgPool>,
/// Built once from the blob-reference registry so this recompute and
@@ -100,6 +124,24 @@ pub struct ManifestsConsistencyCheck {
/// identically. Assembled at construction rather than per page so the
/// sweep runs a fixed statement.
page_sql: String,
/// Repair statement — built from the SAME registry as `page_sql` so
/// detection and repair use identical formulas by construction. Any
/// future 4th manifest-level ref source added to the registry
/// automatically flows into both queries with no code change here.
///
/// Previously the repair query was inlined with the files-only
/// formula, which meant drift from `content_derived_blobs` or
/// `file_attached_blobs` would be DETECTED but NOT repaired even
/// under `?repair=true`. Operators who added those tables saw
/// findings that couldn't be cleared by the repair path — bug fixed
/// 2026-09-02.
///
/// The `?repair=true` gate on the trigger endpoint still stands as
/// the operator's explicit opt-in — this fix only widens what
/// repair CAN do when the operator chooses to run it. Discovery-
/// only remains the default so leaks in insert paths still surface
/// via findings between repair invocations.
repair_sql: String,
}
impl ManifestsConsistencyCheck {
@@ -107,6 +149,7 @@ impl ManifestsConsistencyCheck {
Self {
pool,
page_sql: manifest_page_sql(&reference_registry),
repair_sql: manifest_repair_sql(&reference_registry),
}
}
@@ -308,25 +351,17 @@ impl RecoverableJobHandler for ManifestsConsistencyCheck {
}
finding_count += 1;
let delta = row.actual_ref_count - row.ref_count as i64;
record_or_log(
store,
MANIFESTS_CONSISTENCY_JOB_NAME,
"manifest_refcount_mismatch",
"inconsistent",
None, // a hash isn't a UUID; the identifier lives in detail
serde_json::json!({
"file_hash": row.file_hash,
"stored": row.ref_count,
"actual": row.actual_ref_count,
"delta": delta,
"total_size": row.total_size,
"chunk_count": row.chunk_count,
// Under-count is the dangerous direction: GC reaps a
// manifest whose content is still reachable.
"reap_risk": delta > 0,
}),
)
.await;
let detail = serde_json::json!({
"file_hash": row.file_hash,
"stored": row.ref_count,
"actual": row.actual_ref_count,
"delta": delta,
"total_size": row.total_size,
"chunk_count": row.chunk_count,
// Under-count is the dangerous direction: GC reaps
// a manifest whose content is still reachable.
"reap_risk": delta > 0,
});
// Repair pass — content-safe corrective UPDATE. The
// stored counter is set to what the auditor formula
@@ -338,22 +373,30 @@ impl RecoverableJobHandler for ManifestsConsistencyCheck {
// The `<> (subquery)` guard makes the UPDATE a no-op
// if the value is already correct, so this is
// idempotent under retry.
if repair {
match sqlx::query(
"UPDATE storage.chunk_manifests m \
SET ref_count = ( \
SELECT COUNT(*) FROM storage.files \
WHERE blob_hash = m.file_hash \
) \
WHERE m.file_hash = $1 \
AND m.ref_count <> ( \
SELECT COUNT(*) FROM storage.files \
WHERE blob_hash = m.file_hash \
)",
)
.bind(&row.file_hash)
.execute(self.pool.as_ref())
.await
//
// `self.repair_sql` is built once at construction from
// the same `BlobReferenceRegistry` as the page query —
// detection and repair use identical formulas by
// construction. See `manifest_repair_sql` for the SQL.
//
// Attempt repair FIRST, then record the finding with
// severity/kind reflecting the final state:
// * repair succeeded → severity "info", kind "manifest_refcount_repaired"
// * repair no-op → severity "info", kind "manifest_refcount_resolved"
// * repair failed → severity "inconsistent", kind "manifest_refcount_mismatch"
// * no repair requested → severity "inconsistent", kind "manifest_refcount_mismatch"
//
// Parallels the WARN-then-INFO sequence in logs: an
// unresolved drift raises attention ("inconsistent"),
// a repaired one records the fix at info level without
// inflating the "needs action" tally the outcome UI
// shows. The detail JSON still carries `stored/actual/
// delta` so the audit trail is complete either way.
let (kind, severity) = if repair {
match sqlx::query(&self.repair_sql)
.bind(&row.file_hash)
.execute(self.pool.as_ref())
.await
{
Ok(res) if res.rows_affected() > 0 => {
repaired_count += 1;
@@ -366,12 +409,15 @@ impl RecoverableJobHandler for ManifestsConsistencyCheck {
actual = row.actual_ref_count,
"🩹 manifest ref_count repaired"
);
("manifest_refcount_repaired", "info")
}
Ok(_) => {
// Row not touched — either another concurrent
// repair fixed it first, or the drift healed
// itself between page fetch and UPDATE.
// Silent no-op.
// Row not touched — either another
// concurrent repair fixed it first, or the
// drift healed itself between page fetch
// and UPDATE. Either way, current state
// is correct — record as info.
("manifest_refcount_resolved", "info")
}
Err(e) => {
tracing::warn!(
@@ -382,9 +428,22 @@ impl RecoverableJobHandler for ManifestsConsistencyCheck {
error = %e,
"manifest ref_count repair UPDATE failed — finding stays"
);
("manifest_refcount_mismatch", "inconsistent")
}
}
}
} else {
("manifest_refcount_mismatch", "inconsistent")
};
record_or_log(
store,
MANIFESTS_CONSISTENCY_JOB_NAME,
kind,
severity,
None, // a hash isn't a UUID; the identifier lives in detail
detail,
)
.await;
}
// Advance cursor + checkpoint.
@@ -475,4 +534,33 @@ mod tests {
fn empty_registry_refuses_to_build_page_statement() {
let _ = manifest_page_sql(&BlobReferenceRegistry::new());
}
/// Golden test — the repair statement is assembled from the same
/// registry as `manifest_page_sql`, so pin it byte-for-byte too.
/// If the registry ever changes what it produces at
/// `RefLevel::Manifest`, BOTH this test and
/// `manifest_page_statement_is_stable` above break together — an
/// operator using `?repair=true` shouldn't see the detection
/// formula report drift the repair formula can't clear.
///
/// Ships the three-term formula (`storage.files` +
/// `storage.content_derived_blobs` + `storage.file_attached_blobs`)
/// twice — once in SET, once in the `<>` guard. Both must stay
/// identical so the guard is meaningful (else the UPDATE would fire
/// on drift the SET doesn't fix).
#[tokio::test]
async fn manifest_repair_statement_is_stable() {
let sql = manifest_repair_sql(&default_registry());
let expected = r#"UPDATE storage.chunk_manifests m
SET ref_count = ((SELECT COUNT(*) FROM storage.files cnt_f
WHERE cnt_f.blob_hash = m.file_hash)
+ (SELECT COUNT(*) FROM storage.content_derived_blobs cnt_d WHERE cnt_d.blob_hash = m.file_hash)
+ (SELECT COUNT(*) FROM storage.file_attached_blobs cnt_a WHERE cnt_a.blob_hash = m.file_hash))::bigint
WHERE m.file_hash = $1
AND m.ref_count <> ((SELECT COUNT(*) FROM storage.files cnt_f
WHERE cnt_f.blob_hash = m.file_hash)
+ (SELECT COUNT(*) FROM storage.content_derived_blobs cnt_d WHERE cnt_d.blob_hash = m.file_hash)
+ (SELECT COUNT(*) FROM storage.file_attached_blobs cnt_a WHERE cnt_a.blob_hash = m.file_hash))::bigint"#;
assert_eq!(sql, expected, "manifest repair statement changed:\n{sql}");
}
}