fix(files): make file writes transactional and close a quota bypass
Audit #6 and #8 for the last of the three stores, plus a bypass found on the way. **#6.** create/update/delete wrote the metadata row, then emitted best-effort, so an outbox failure left a committed file whose trigger never fired. `atomic_write::FilesWriter` commits the metadata row and the fan-out together. Files are the one store where the ordering is subtle, because the BYTES live on disk and cannot join a transaction: * create/update — blob first, then commit metadata + fan-out. A rollback unlinks the blob. (A crash at that exact point still orphans it; that hazard predates this change — the repo already wrote the blob and then inserted the row in a separate, failable statement — and the orphan is inert, referenced by nothing.) * delete — commit the metadata removal + fan-out FIRST, then unlink. The reverse order would destroy the bytes of a row that a rollback keeps, leaving a file that can never be read. **#8.** `GroupFilesService::create` read `total_bytes` on one connection and wrote on another, so concurrent uploads each saw the same pre-write total and together overshot the ceiling. This is the worst instance of the race in the codebase: the ceiling is DISK (10 GiB by default) and one file may be 100 MB, so a racing fleet overshoots by gigabytes. `PostgresGroupFilesWriter` takes the per-group advisory lock (on its own `files` key) across the check and the write. **The bypass.** `GroupFilesService::update` checked NO quota at all — so a 1-byte file could be updated to a 100 MB one without the ceiling ever being consulted, repeatedly, for unbounded disk. It now checks the projected total (the replaced file's bytes subtracted in SQL, so a same-size-or-smaller update near the cap still goes through). Also drive-by: `queue_e2e` asserted the ack the instant the marker appeared, but the marker is written DURING the handler and the ack happens after it returns — a zero-tolerance race. It polls now. (This does not fix the suite's flakiness, which reproduces on the pre-pass commit too.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -16,12 +16,14 @@
|
||||
use std::sync::Arc;
|
||||
|
||||
use picloud_manager_core::atomic_write::{
|
||||
GroupDocsTarget, GroupDocsWriter, GroupKvTarget, GroupKvWriter, KvWriter,
|
||||
PostgresGroupDocsWriter, PostgresGroupKvWriter, PostgresKvWriter,
|
||||
GroupDocsTarget, GroupDocsWriter, GroupFilesQuota, GroupFilesWriter, GroupKvTarget,
|
||||
GroupKvWriter, KvWriter, PostgresGroupDocsWriter, PostgresGroupFilesWriter,
|
||||
PostgresGroupKvWriter, PostgresKvWriter,
|
||||
};
|
||||
use picloud_manager_core::group_quota::GroupWriteQuota;
|
||||
use picloud_shared::{
|
||||
AppId, ExecutionId, GroupDocsError, GroupId, GroupKvError, RequestId, ScriptId, SdkCallCx,
|
||||
AppId, ExecutionId, FileUpdate, GroupDocsError, GroupFilesError, GroupId, GroupKvError,
|
||||
NewFile, RequestId, ScriptId, SdkCallCx,
|
||||
};
|
||||
use sqlx::postgres::PgPoolOptions;
|
||||
use sqlx::PgPool;
|
||||
@@ -503,3 +505,106 @@ async fn concurrent_writers_cannot_push_a_group_past_its_docs_row_quota() {
|
||||
.await
|
||||
.expect("cleanup");
|
||||
}
|
||||
|
||||
/// Group FILES had the worst version of the race: the ceiling is disk (10 GiB by
|
||||
/// default) and a single file may be 100 MB, so a fleet of concurrent uploads
|
||||
/// each seeing the same pre-write total could overshoot by GIGABYTES of real
|
||||
/// disk. Same fix — a per-group advisory lock across the check and the write.
|
||||
///
|
||||
/// It also pins the second half of the bug: `update` checked NO quota at all, so
|
||||
/// a 1-byte file could be grown past the ceiling unchallenged.
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn concurrent_uploads_cannot_push_a_group_past_its_files_byte_quota() {
|
||||
let Some(pool) = pool_or_skip().await else {
|
||||
return;
|
||||
};
|
||||
let group = mk_group(&pool).await;
|
||||
let root = std::env::temp_dir().join(format!("picloud-aw-{}", Uuid::new_v4().simple()));
|
||||
let writer = Arc::new(PostgresGroupFilesWriter::new(pool.clone(), root.clone()));
|
||||
|
||||
// Ten 100-byte uploads race for a 450-byte ceiling: at most 4 may land.
|
||||
let quota = GroupFilesQuota {
|
||||
max_total_bytes: 450,
|
||||
};
|
||||
let app = Uuid::new_v4();
|
||||
let mut set = tokio::task::JoinSet::new();
|
||||
for i in 0..10 {
|
||||
let w = Arc::clone(&writer);
|
||||
set.spawn(async move {
|
||||
let cx = cx(app);
|
||||
w.create(
|
||||
&cx,
|
||||
GroupId::from(group),
|
||||
"assets",
|
||||
NewFile {
|
||||
name: format!("f{i}.bin"),
|
||||
content_type: "application/octet-stream".into(),
|
||||
data: vec![b'x'; 100],
|
||||
},
|
||||
quota,
|
||||
)
|
||||
.await
|
||||
});
|
||||
}
|
||||
let mut created = Vec::new();
|
||||
while let Some(r) = set.join_next().await {
|
||||
match r.expect("task") {
|
||||
Ok(id) => created.push(id),
|
||||
Err(GroupFilesError::QuotaExceeded { .. }) => {}
|
||||
Err(e) => panic!("unexpected error: {e}"),
|
||||
}
|
||||
}
|
||||
|
||||
let stored = |pool: PgPool| async move {
|
||||
let (n,): (i64,) = sqlx::query_as(
|
||||
"SELECT COALESCE(SUM(size_bytes), 0)::BIGINT FROM group_files WHERE group_id = $1",
|
||||
)
|
||||
.bind(group)
|
||||
.fetch_one(&pool)
|
||||
.await
|
||||
.expect("bytes");
|
||||
n
|
||||
};
|
||||
let bytes = stored(pool.clone()).await;
|
||||
assert!(
|
||||
bytes <= 450,
|
||||
"stored bytes ({bytes}) must not exceed the 450-byte ceiling — concurrent \
|
||||
uploads must not each see the same pre-write total"
|
||||
);
|
||||
assert!(!created.is_empty(), "some uploads should have succeeded");
|
||||
|
||||
// An UPDATE that would blow the ceiling must now be refused too. Previously
|
||||
// update consulted no quota at all, so this was a free bypass.
|
||||
let victim = created[0];
|
||||
let err = writer
|
||||
.update(
|
||||
&cx(app),
|
||||
GroupId::from(group),
|
||||
"assets",
|
||||
victim,
|
||||
FileUpdate {
|
||||
name: None,
|
||||
content_type: None,
|
||||
data: vec![b'y'; 10_000],
|
||||
},
|
||||
quota,
|
||||
)
|
||||
.await
|
||||
.expect_err("growing a file past the group ceiling must be refused");
|
||||
assert!(
|
||||
matches!(err, GroupFilesError::QuotaExceeded { .. }),
|
||||
"got {err}"
|
||||
);
|
||||
assert_eq!(
|
||||
stored(pool.clone()).await,
|
||||
bytes,
|
||||
"the refused update must not have changed the stored total"
|
||||
);
|
||||
|
||||
sqlx::query("DELETE FROM groups WHERE id = $1")
|
||||
.bind(group)
|
||||
.execute(&pool)
|
||||
.await
|
||||
.expect("cleanup");
|
||||
let _ = std::fs::remove_dir_all(&root);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user