-
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 4 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,47 @@ 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) { | ||
| // Deleting the current default branch would leave the repository with a | ||
| // dangling HEAD: ref discovery advertises a zero id and import APIs unwrap | ||
| // the now-missing default ref. Reject it for every path (deletion-only and | ||
| // mixed pushes alike) before any ref row is removed. | ||
| if let Some(default_ref) = storage | ||
| .git_db_storage() | ||
| .get_ref(repo_id) | ||
| .await? | ||
| .into_iter() | ||
| .find(|r| r.default_branch) | ||
| && commands.iter().any(|c| { | ||
| c.ref_type == RefTypeEnum::Branch | ||
| && c.command_type == CommandType::Delete | ||
| && c.ref_name == default_ref.ref_name | ||
| }) | ||
| { | ||
| return Err(MegaError::Other(format!( | ||
| "cannot delete the current default branch {}", | ||
| default_ref.ref_name | ||
| ))); | ||
| } | ||
|
|
||
| // 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. | ||
| let branch_cmds: Vec<&RefCommand> = commands | ||
| .iter() | ||
| .filter(|c| c.ref_type == RefTypeEnum::Branch) | ||
| .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; | ||
|
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 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 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, 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). |
||
| } | ||
| }; | ||
|
|
||
| let mono_storage = storage.mono_storage(); | ||
|
|
@@ -192,3 +230,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,45 @@ 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 | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| // 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 +293,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 +327,9 @@ impl SmartSession { | |
|
|
||
| let mut finalize_ms: Option<u128> = None; | ||
| let mut bind_ms: Option<u128> = None; | ||
| if !unpack_failed { | ||
| // Nothing left to persist when every command was rejected up front | ||
| // (e.g. a delete-only monorepo push); skip finalize and just report. | ||
| if !unpack_failed && commands.iter().any(|c| c.status == "ok") { | ||
|
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 branch deletion on an import repository, the command remains 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, and it is the import-side twin of the monorepo finding: pkt-line framing now delivers pack-less pushes to finalization, where dispatch_import_receive_pack_finalized resolved the first branch command's new_id as a commit. A deletion's new_id is the zero id, so the lookup failed with "commit 000... not found" before remove_ref_in_txn could run — and any mixed push whose first branch command happened to be a deletion hit the same failure despite carrying a valid tip. Fixed in 2ef1343:
Note traverses_tree_and_update_filepath already handled this correctly (it filters c.new_id != ZERO_ID and falls back to DB HEAD), which is why only the dispatch step needed the change. 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). 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.
Now that pack-less bodies are processed, a normal tag-only create/update targeting an object already present in a monorepo reaches this condition with an 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, and worth noting the pack-less framing only exposed a pre-existing failure: even before this PR, any tag-only monorepo push (pack included) reached finalize with an empty to_hash and died in the MonoReceivePackFinalized event, because sync_cl_ref rejects an empty hash. Pack-less pushes just made it reachable without unpacking first. Fixed in d4e03de: monorepo finalization now runs only when a surviving branch command exists. Tag-only monorepo pushes therefore complete with their report instead of failing after update_refs already materialized the ref; delete-only rejection handling is unchanged (no surviving branch command there either). Import finalization is untouched — its dispatch returns early without branch commands and its filepath refresh still runs for tag-only pushes. A deeper question — what a tag push should mean for CL state on a CL-based monorepo (MonoRepo::update_refs currently materializes refs/cl/{link} for tags too) — is existing behavior this PR deliberately does not redesign. 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). |
||
| 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 push combines a tag create/update/delete with deletion of the current default branch, the tag is already persisted by
update_refsinsmart.rs:295-303before finalization reaches this new rejection. The error then returns immediately fromgit_receive_pack_streamwithout a per-ref status report, so the client sees the push fail even though the tag change remains committed. Perform this validation before applying tag commands, or include the tag and branch changes in one transaction.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 ordering flaw: tags were persisted by update_refs before finalization reached the default-branch rejection, so failing there left committed tag rows behind a failed push with no per-ref report.
Fixed in e9ff80c by moving the validation earlier, into import handler construction in repo_handler_with_commands: a push deleting the current default branch now aborts with ProtocolError::InvalidInput before any persistence at all — no tag writes, no ref changes, no attach work. The late guard in dispatch was removed as redundant; there is now exactly one choke point.
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).