-
Notifications
You must be signed in to change notification settings - Fork 122
fix(git-protocol): frame receive-pack bodies by pkt-line structure #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
1c0c40a
bb956dd
2ef1343
3345e3b
e9ff80c
d4e03de
83dc33c
edb3a7c
f5b0d42
374d7ac
e8fb6dc
5258883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,27 @@ pub async fn dispatch_import_receive_pack_finalized( | |
| unpack_redlock: Arc<RedLock>, | ||
| extra_timings: Arc<Mutex<Vec<(String, u128)>>>, | ||
| ) -> Result<(), MegaError> { | ||
| let commit_id = match commands.iter().find(|c| c.ref_type == RefTypeEnum::Branch) { | ||
| // The attach commit is sourced from the pushed branch tip; deletions carry | ||
| // the zero id and resolve no commit. A push whose branch commands are all | ||
| // deletions has no content to attach (the repo is necessarily attached | ||
| // already, or its refs would not exist), so just apply the deletions. | ||
| // Commands already rejected by the protocol layer keep their ng status and | ||
| // are excluded here; their report lines were already emitted upstream. | ||
| let branch_cmds: Vec<&RefCommand> = commands | ||
| .iter() | ||
| .filter(|c| c.ref_type == RefTypeEnum::Branch && c.status == "ok") | ||
| .collect(); | ||
| let attach_source = branch_cmds | ||
| .iter() | ||
| .find(|c| c.command_type != CommandType::Delete); | ||
| let commit_id = match attach_source { | ||
| Some(cmd) => cmd.new_id.clone(), | ||
| None => return Ok(()), | ||
| None => { | ||
| if branch_cmds.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| return apply_branch_deletions(&storage, repo_id, &branch_cmds).await; | ||
| } | ||
| }; | ||
|
|
||
| let mono_storage = storage.mono_storage(); | ||
|
|
@@ -83,10 +101,7 @@ pub async fn dispatch_import_receive_pack_finalized( | |
|
|
||
| let txn = storage.begin_db_transaction().await?; | ||
| let git_db = storage.git_db_storage(); | ||
| for cmd in &commands { | ||
| if cmd.ref_type != RefTypeEnum::Branch { | ||
| continue; | ||
| } | ||
| for &cmd in &branch_cmds { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an import push lists a nondefault deletion before a valid branch update, the new attach-source selection skips the deletion and lets this mixed-command loop run; its Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid: the mixed-command loop's Delete arm removed by name only, bypassing the lease validation used by the deletion-only helper. Fixed in 374d7ac: both paths now share the same atomic conditional delete (GitDbStorage::remove_ref_if_unchanged, keyed on the advertised old id); a mismatch inside the attach transaction returns an error that rolls the whole transaction back, so a moved ref is never removed and the push reports failure instead of false success. Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| match cmd.command_type { | ||
| CommandType::Create => { | ||
| git_db | ||
|
|
@@ -192,3 +207,20 @@ pub async fn dispatch_import_receive_pack_finalized( | |
| "attach_to_monorepo_parent: exceeded retry limit for concurrent root updates".into(), | ||
| )) | ||
| } | ||
|
|
||
| /// Applies deletion-only branch commands. The monorepo root ref is untouched, | ||
| /// so the root update lock and attach-retry loop are not needed. | ||
| async fn apply_branch_deletions( | ||
| storage: &Storage, | ||
| repo_id: i64, | ||
| deletions: &[&RefCommand], | ||
| ) -> Result<(), MegaError> { | ||
| let txn = storage.begin_db_transaction().await?; | ||
| let git_db = storage.git_db_storage(); | ||
| for cmd in deletions { | ||
| git_db | ||
| .remove_ref_in_txn(repo_id, &cmd.ref_name, &txn) | ||
| .await?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less deletion of a nondefault import branch races with another push after ref advertisement, this removes the ref solely by name and never compares its current value with Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid race: deleting by name alone honors neither the client'"'"'s advertised old id nor any lease semantics, so a stale push could remove another user'"'"'s newer tip. Fixed in f5b0d42: apply_branch_deletions now does compare-and-delete within the transaction — a new GitDbStorage::get_ref_by_name_in_txn reads the current value under the same txn that deletes, and a mismatch with cmd.old_id aborts the whole deletion batch with "ref moved since advertisement (expected ..., found ...)", which surfaces as an ng report for the deletion commands. Nothing is removed on mismatch. One honest adjacent note: the Create/Update arms of the attach transaction use unconditional save/update_ref_in_txn, so the same stale-write hazard exists there for non-delete pushes. That behavior predates this PR (it applies to ordinary pack-carrying pushes) and touches the attach flow broadly, so I left it alone rather than widen this PR; happy to follow up separately if maintainers want CAS there too. Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean; cargo clippy -p jupiter --all-targets: clean). |
||
| } | ||
| txn.commit().await.map_err(MegaError::Db) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ use crate::{ | |
| infra::cache::GitObjectCache, | ||
| transport::{ | ||
| pack::RepoHandler, | ||
| protocol::import_refs::{RefCommand, Refs}, | ||
| protocol::import_refs::{CommandType, RefCommand, Refs}, | ||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -479,7 +479,7 @@ impl MonoRepo { | |
| .clone(); | ||
| let txn = self.storage.begin_db_transaction().await?; | ||
| for cmd in &cmds { | ||
| if cmd.ref_type == RefTypeEnum::Branch { | ||
| if cmd.ref_type == RefTypeEnum::Branch && cmd.command_type != CommandType::Delete { | ||
| self.apply_cl_mega_ref_for_push_command(cmd, Some(&txn)) | ||
|
Comment on lines
523
to
528
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less monorepo push creates or updates two branches to commits the server already has, this loop reports both commands successful but applies both through the same Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid. Pack-less multi-branch updates bypassed check_entry'"'"'s single-commit restriction (it only runs during unpack), so both commands reported ok while materializing through one shared refs/cl/ ref. Fixed in e8fb6dc: the monorepo up-front guard now enforces at most one surviving branch update per push, matching the documented invariant ("only single commit support in each push") that packed pushes already hit in check_entry. Additional branch commands are rejected with an explicit ng before anything is persisted. Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| .await?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less monorepo request mixes a missing-target branch update with a valid update to an existing commit, validation marks only the former Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid: the persistence transaction filtered deletions but not status, so [missing-target update (ng), valid update (ok)] aborted the whole transaction on the missing commit and took the valid update down with it. Fixed in edb3a7c: persist_mono_branch_cl_mega_refs_transaction now processes only branch commands whose status is still ok, so each surviving update lands independently of rejected siblings. The same status-blindness existed in two more consumers reached by the same push shapes, fixed in the same commit: the import traversal's tip selection (also ignored status) and monorepo tip metadata (base_branch/from_hash/to_hash), which was captured at handler construction before validation ran — it now lives behind a mutex and is re-derived from surviving commands in sync_commands_after_unpack, so MonoReceivePackFinalized can no longer carry a rejected command's hash. Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo test -p ceres --lib pack: 4 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ use crate::{ | |
| pack::PackByteStream, | ||
| protocol::{ | ||
| Capability, ServiceType, SideBind, SmartSession, TransportProtocol, ZERO_ID, | ||
| import_refs::RefCommand, | ||
| import_refs::{CommandType, RefCommand}, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
@@ -226,7 +226,7 @@ impl SmartSession { | |
| &mut self, | ||
| state: &TransportRuntime, | ||
| commands: Vec<RefCommand>, | ||
| data_stream: PackByteStream, | ||
| pack_stream: Option<PackByteStream>, | ||
| ) -> Result<Bytes, ProtocolError> { | ||
| let t0 = Instant::now(); | ||
| let mut timings_ms: BTreeMap<String, u128> = BTreeMap::new(); | ||
|
|
@@ -238,21 +238,60 @@ impl SmartSession { | |
| .repo_handler_with_commands(state, commands.clone()) | ||
| .await?; | ||
| let is_monorepo = repo_handler.is_monorepo(); | ||
| //1. unpack progress | ||
| // A deletion's new id is the zero id and monorepo state only advances | ||
| // through CL merges, so there is no commit to materialize a ref update | ||
| // from. Reject such commands up front (import repos do support | ||
| // deletion) instead of failing later with opaque zero-id lookup errors. | ||
| if is_monorepo { | ||
| for command in commands.iter_mut() { | ||
| if command.command_type == CommandType::Delete { | ||
|
Comment on lines
+247
to
+249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a monorepo push lists a branch deletion before another valid branch update, Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid. repo_handler_with_commands copied base_branch/from_hash/to_hash from the first branch command even when that command was a rejected deletion, so a mixed push [delete foo, update main] persisted the surviving CL ref while MonoReceivePackFinalized still carried foo's zero target and sync_cl_ref failed it. Fixed in e9ff80c at the source: monorepo metadata is now sourced from the first non-deletion branch command. For mixed pushes the finalize event describes the real update; for delete-only pushes no command matches and metadata stays empty (finalize is skipped anyway since every command is rejected). This also covers pack-carrying delete-first pushes, where the same mis-sourcing existed before this PR. Verified in Docker (cargo test -p ceres --lib transport::protocol: 9 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| command.failed(format!( | ||
| "deleting {} is not supported on monorepo", | ||
| command.ref_name | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| // A pack-less push carries no objects, so any surviving non-deletion | ||
| // command must target an object the server already stores; real git | ||
| // clients never send otherwise ("everything up-to-date" sends no | ||
| // request at all). Fail such commands here rather than letting | ||
| // finalization trip over the missing object later. | ||
| if pack_stream.is_none() { | ||
| for command in commands.iter_mut() { | ||
| if command.command_type != CommandType::Delete && command.status == "ok" { | ||
| let exists = repo_handler.check_commit_exist(&command.new_id).await; | ||
| if !exists { | ||
| command.failed(format!("target object {} not found", command.new_id)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less import tag create/update points to an annotated-tag object already stored in the repository, this calls Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid. The commit-only existence check would falsely reject a pack-less annotated-tag command whose tag object is legitimately stored, since check_commit_exist only queries the commits table. Fixed in f5b0d42 per your first suggested option: the pack-less validation now applies to branch commands only. Tags are exempt from it — real git clients cannot produce a pack-less annotated-tag create anyway (the tag object must travel in a pack unless everything including that exact object already exists, in which case git sends no request at all). Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| } | ||
| } | ||
| } | ||
| } | ||
| // 1. unpack progress. Pack-less pushes (e.g. ref deletions) carry no packfile | ||
| // after the command flush, so unpack is skipped entirely. | ||
| let t_unpack = Instant::now(); | ||
| let receiver = repo_handler | ||
| .unpack_stream(&state.storage.config().pack, data_stream) | ||
| .await?; | ||
| let receiver = match pack_stream { | ||
| Some(stream) => Some( | ||
| repo_handler | ||
| .unpack_stream(&state.storage.config().pack, stream) | ||
| .await?, | ||
| ), | ||
| None => None, | ||
| }; | ||
| timings_ms.insert( | ||
| "unpack_stream_ms".to_string(), | ||
| t_unpack.elapsed().as_millis(), | ||
| ); | ||
|
|
||
| let t_receiver = Instant::now(); | ||
| let unpack_result = repo_handler | ||
| .clone() | ||
| .receiver_handler(receiver.0, receiver.1) | ||
| .await; | ||
| let unpack_result = if let Some((receiver, rx_pack_id)) = receiver { | ||
| repo_handler | ||
| .clone() | ||
| .receiver_handler(receiver, rx_pack_id) | ||
| .await | ||
| } else { | ||
| Ok(()) | ||
|
Comment on lines
+323
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less push deletes a branch in a monorepo, this treats the skipped unpack as successful and proceeds into Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid finding. With pkt-line framing in place, a delete-only push reaches finalize_receive_pack, where apply_cl_mega_ref_for_push_command looks up the all-zero new_id as a commit and aborts the whole push with an opaque "Commit 0000... not found while writing CL ref" error. Fixed in bb956dd:
We deliberately did not implement actual branch deletion for monorepos in this PR: mono branch state advances only through CL merges, and real deletion support would require changes to the CL/post-receive flow (update_or_create_cl, build triggers, review reanchoring). An explicit, well-reported rejection is the honest behavior until that exists; the previous failure mode looked like a server bug. Verified in Docker (cargo test -p ceres --lib transport::protocol: 9 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean).
Comment on lines
+323
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less request contains a branch create/update whose Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid: a crafted pack-less body with an unstored new_id panicked the import handler at traverses_tree_and_update_filepath (unwrap on a missing commit) instead of reporting ng. Fixed in 83dc33c, two parts:
Verified in Docker (cargo test -p ceres --lib transport::protocol: 9 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| }; | ||
| timings_ms.insert( | ||
| "receiver_handler_ms".to_string(), | ||
| t_receiver.elapsed().as_millis(), | ||
|
|
@@ -269,7 +308,11 @@ impl SmartSession { | |
| // mono and import both persist branch refs inside `finalize_receive_pack`. | ||
| for command in commands.iter_mut() { | ||
| if command.ref_type == RefTypeEnum::Tag { | ||
| // just update if refs type is tag | ||
| // Already-rejected commands (e.g. monorepo deletions) keep | ||
| // their up-front failure reason instead of being re-processed. | ||
| if command.status != "ok" { | ||
| continue; | ||
| } | ||
| if let Err(e) = repo_handler.update_refs(command).await { | ||
| command.failed(e.to_string()); | ||
|
Comment on lines
347
to
348
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pack-less import push combines a tag mutation with a stale deletion of a nondefault branch, this eagerly commits the tag before branch finalization; the conditional branch deletion then fails and line 391 returns a protocol error, leaving the tag changed even though the push is reported as failed. Fresh evidence beyond the default-branch precheck is the newly added Useful? React with 👍 / 👎. |
||
| } | ||
|
|
@@ -299,7 +342,15 @@ impl SmartSession { | |
|
|
||
| let mut finalize_ms: Option<u128> = None; | ||
| let mut bind_ms: Option<u128> = None; | ||
| if !unpack_failed { | ||
| // Skip finalize when nothing survives to finalize: every command | ||
| // rejected up front (e.g. delete-only monorepo pushes), or a monorepo | ||
| // push carrying only tag mutations — monorepo finalize events describe | ||
| // a branch tip and fail against an empty one. Import finalization is | ||
| // safe without branches (its dispatch returns early) and keeps running. | ||
| let has_branch_work = commands | ||
| .iter() | ||
| .any(|c| c.ref_type == RefTypeEnum::Branch && c.status == "ok"); | ||
| if !unpack_failed && (!is_monorepo || has_branch_work) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a pack-less import request whose non-deletion target is absent, the new validation marks the command Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed — valid. The finalize gate still exempted import repos, so a pack-less request whose only branch command failed target validation ran traverses_tree_and_update_filepath, which selected that failed command without checking its status and unwrapped its missing commit. Fixed in edb3a7c: finalization now requires a surviving ok branch command for BOTH repo types — an import push with only rejected commands (or only tag mutations) is skipped entirely and reports cleanly. Defense in depth was added too: the traversal's tip selection now ignores commands whose status is not ok. One honest correction to my earlier reply on 3855694878: I had said import finalization "keeps running" for tag-only pushes to preserve its filepath refresh; after adding target validation that unconditional path became this panic vector, so uniform gating won. Tag-only import pushes now skip finalize like monorepo ones. Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo test -p ceres --lib pack: 4 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean). |
||
| let t_finalize = Instant::now(); | ||
| if let Err(e) = repo_handler.finalize_receive_pack().await { | ||
| let msg = e.to_string(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an import repository has another branch and a push deletes its current default branch, this deletion-only path removes the default row without promoting another branch or rejecting the operation. Because
check_default_branchran before deletion, no replacement is marked; subsequent ref discovery advertises a zeroHEAD, and import APIs such asget_root_commitunwrap the now-missing default ref. Either reject deletion of the default branch or select a replacement transactionally.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed — valid, and newly reachable through the deletion-only path introduced by 2ef1343: nothing stopped that transaction from removing the default row, leaving ref discovery advertising a zero HEAD and import APIs unwrapping the missing default ref.
Fixed in 3345e3b: dispatch_import_receive_pack_finalized now refuses any push whose branch commands delete the current default ref ("cannot delete the current default branch refs/heads/x") before any row is touched. The guard sits above both paths, which also closes the pre-existing hole where a mixed push could delete the default row via the attach transaction without promoting a replacement.
Two honest scope notes:
Verified in Docker (cargo test -p ceres --lib transport::protocol: 9 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean).