fix(git-protocol): frame receive-pack bodies by pkt-line structure - #2174
Conversation
Locating the pack section by scanning raw request bytes for the "PACK" magic fails in three ways: - A push that carries no packfile at all (e.g. ref deletion) never matches, so the whole body is drained silently and an empty report is returned while refs stay unprocessed. - The 4-byte signature can straddle a transport chunk boundary and go undetected, producing the same empty-report failure. - A ref name containing "PACK" (e.g. refs/heads/JDK-PACK) matches mid-command, truncating the command list inside a pkt-line. Frame the body by protocol structure instead: walk pkt-lines from the buffered chunks until the terminating flush packet, treat everything after it as pack data, and skip unpack entirely for pack-less pushes. SSH receive-pack input goes through the same helper. Verified against real git: the captured body of a delete-only push contains only pkt-line commands plus the flush packet, with no "PACK" magic present. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c0c40ae8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| Ok(()) |
There was a problem hiding this comment.
When a pack-less push deletes a branch in a monorepo, this treats the skipped unpack as successful and proceeds into MonoRepo::finalize_receive_pack; however, apply_cl_mega_ref_for_push_command then looks up cmd.new_id as a commit (monorepo.rs:499-511), while a deletion's new ID is the all-zero object ID. The lookup therefore fails and both HTTP and SSH git push --delete return an error without deleting the ref, which defeats the principal pack-less scenario added by this change. Handle CommandType::Delete explicitly in monorepo finalization rather than treating it as a ref-only update to an existing commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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:
- On the monorepo path, Branch commands with CommandType::Delete are now rejected up front in git_receive_pack_stream with an explicit per-ref message ("deleting refs/heads/x is not supported on monorepo"). Import repos keep their working delete handling (import_repo::update_refs) untouched.
- persist_mono_branch_cl_mega_refs_transaction skips Delete commands, so a mixed push (update + delete) still lands its updates instead of failing the whole transaction on the zero-id lookup.
- Finalize is skipped entirely when no command remains ok, so a delete-only push returns a clean report-status instead of running CL/event side effects against a zero id.
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).
A pack-less delete-only push now survives framing and reaches monorepo finalization, 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' error. Monorepo branch state only advances through CL merges, so a deletion has no commit to materialize a ref update from. Mark Branch+Delete commands failed up front on the monorepo path (import repos keep their working delete support) with a clear per-ref message, skip them when persisting CL mega_refs so mixed pushes still land their updates, and skip finalize entirely when no command remains ok. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb956ddb53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
For a pack-less branch deletion on an import repository, the command remains ok, so this condition calls ImportRepo::finalize_receive_pack. The downstream dispatch_import_receive_pack_finalized selects the first branch command's new_id and requires that commit to exist before processing the deletion; for a delete this is ZERO_ID, so it returns commit 000… not found before remove_ref_in_txn runs. Consequently both HTTP and SSH git push --delete still fail for import repositories; the deletion path must finalize without resolving the zero target as a commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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:
- The attach commit is now sourced from the first non-deletion branch command, so mixed pushes (delete + update, in any order) attach from the real tip while the transaction still applies both the deletions and the updates.
- When every branch command is a deletion there is no content to attach: the repo is necessarily attached already (its refs exist), so the deletions are applied in a plain transaction without acquiring the root update lock or entering the attach-retry loop.
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).
…ttach Pack-less framing routes delete-only pushes on import repositories into dispatch_import_receive_pack_finalized, which resolved the first branch command's new_id as a commit; a deletion's new_id is the all-zero id, so the lookup failed with 'commit 000... not found' before remove_ref_in_txn could run. A mixed push whose first branch command is a deletion hit the same failure even though a later update carried a valid tip. Source the attach commit from the first non-deletion branch command, and when every branch command is a deletion apply them in a plain transaction: no content changed, the repo is necessarily attached already (its refs exist), so neither the root update lock nor the attach-retry machinery is needed. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef1343179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if command.ref_type == RefTypeEnum::Branch | ||
| && command.command_type == CommandType::Delete |
There was a problem hiding this comment.
When a pack-less deletion targets refs/tags/* in a monorepo, this guard does not reject it because it only matches branches. The command then reaches MonoRepo::update_refs, whose apply_cl_mega_ref_for_push_command attempts to resolve the deletion's all-zero new_id as a commit, so the advertised delete-refs operation returns ng and leaves the tag unchanged. Handle tag deletions explicitly before the normal tag update path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — valid. The up-front rejection only matched RefTypeEnum::Branch, so a monorepo tag deletion slipped through to MonoRepo::update_refs, whose apply_cl_mega_ref_for_push_command resolved the all-zero new_id as a commit and returned an opaque ng.
Fixed in 3345e3b: every monorepo deletion (branch or tag) is now rejected up front with the explicit "deleting refs/tags/x is not supported on monorepo" reason, and the tag update loop skips already-rejected commands so they keep that reason instead of being re-processed into the zero-id lookup error. As before, real deletion support for monorepos would require CL-flow changes and is out of scope for 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).
| if branch_cmds.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| return apply_branch_deletions(&storage, repo_id, &branch_cmds).await; |
There was a problem hiding this comment.
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_branch ran before deletion, no replacement is marked; subsequent ref discovery advertises a zero HEAD, and import APIs such as get_root_commit unwrap 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.
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:
- Re-pointing the default deliberately (e.g. promoting another branch) is repository-settings territory, not something receive-pack should guess at, so rejection rather than transactional promotion.
- The rejection surfaces through the existing finalize-error channel, meaning the push fails as a whole with that message rather than as a per-ref ng line — consistent with how other finalize failures (e.g. attach conflicts) already report in this codebase.
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).
…h deletion Two deletion edge cases surfaced by review of the pack-less paths: Monorepo: the up-front rejection only matched Branch commands, so a refs/tags/* deletion slipped through to MonoRepo::update_refs, whose apply_cl_mega_ref_for_push_command resolved the all-zero new_id as a commit and returned an opaque ng. Reject every monorepo deletion up front regardless of ref type, and let already-rejected commands keep their reason instead of being re-processed by the tag update loop. Import: deleting the current default branch (deletion-only push, or as part of a mixed push) removed the default row without promoting a replacement, leaving ref discovery advertising a zero HEAD and import APIs unwrapping the missing ref. Refuse such deletions before any row is touched; users can re-point the default through repository settings instead. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3345e3b9e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if is_monorepo { | ||
| for command in commands.iter_mut() { | ||
| if command.command_type == CommandType::Delete { |
There was a problem hiding this comment.
When a monorepo push lists a branch deletion before another valid branch update, repo_handler_with_commands has already copied that first branch's name and zero new_id into base_branch/to_hash (protocol/mod.rs:234-241). This marks only the deletion failed, so the surviving update still triggers finalization; its CL ref is persisted, but MonoReceivePackFinalized then uses the rejected deletion's zero target and sync_cl_ref rejects that missing commit (application/code_edit/model.rs:442-447), leaving a partial update while the push reports failure. Recompute the handler metadata from a surviving command or reject the mixed push before finalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
| && commands.iter().any(|c| { | ||
| c.ref_type == RefTypeEnum::Branch | ||
| && c.command_type == CommandType::Delete | ||
| && c.ref_name == default_ref.ref_name | ||
| }) |
There was a problem hiding this comment.
When an import push combines a tag create/update/delete with deletion of the current default branch, the tag is already persisted by update_refs in smart.rs:295-303 before finalization reaches this new rejection. The error then returns immediately from git_receive_pack_stream without 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.
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).
Two ordering flaws remained in the deletion handling: Monorepo metadata (base_branch/from_hash/to_hash) was copied from the first branch command even when that command was a deletion, so a mixed push [delete foo, update main] finalized its surviving CL ref while the post-receive event still carried the rejected deletion's zero target and failed. Metadata is now sourced from the first non-deletion branch command. The import default-branch rejection ran inside finalization, after tag commands had already been persisted by update_refs; failing there left committed tags behind a failed push. The check moved to import handler construction, where an early InvalidInput aborts before any persistence, and the now-redundant late guard was removed. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9ff80cf8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
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 ok command. MonoRepo::update_refs has already written its CL ref, but repo_handler_with_commands only initializes from_hash/to_hash from branch commands, so finalize_receive_pack dispatches MonoReceivePackFinalized with an empty to_hash; CL synchronization then rejects that hash and the client sees a failed push after the ref mutation has persisted. Exclude tag-only command sets from monorepo branch finalization, or supply a valid finalization target and make the mutation atomic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
A pack-less tag-only push on a monorepo now reaches finalization, where the MonoReceivePackFinalized event carries an empty to_hash (no branch commands) and sync_cl_ref fails with 'invalid to_hash', failing the push after the tag ref was already materialized by update_refs. The same failure mode existed for pack-carrying tag-only pushes, which always died in the finalize event. Monorepo finalization drives CL/branch state, so it now runs only when a surviving branch command exists. Import finalization is unaffected: its dispatch returns early without branch commands and its filepath refresh keeps running for tag-only pushes. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e03de1ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| Ok(()) |
There was a problem hiding this comment.
When a pack-less request contains a branch create/update whose new_id is not already stored, this treats the skipped unpack as successful and sends the command into import finalization. ImportRepo::traverses_tree_and_update_filepath then unwraps the missing commit returned by get_commit_by_hash (import_repo.rs:414-419), so a malformed or incomplete receive-pack request drops the connection via panic instead of returning an ng status. Before returning Ok(()) for a missing pack, verify every surviving non-delete target exists and reject commands whose objects are unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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:
- For pack-less requests, every surviving non-deletion command is now validated against the server's object store via RepoHandler::check_commit_exist before finalization; missing targets are rejected with "target object not found". This is safe for real clients: when everything is up to date git sends no request at all, so a non-deletion command without a pack only occurs in malformed or hostile requests. It covers both repo types (the monorepo path already handled missing commits gracefully but now fails per-command up front).
- Import dispatch now considers only branch commands that survived validation, both when sourcing the attach commit and inside the attach transaction, so a rejected command can neither be selected as the attach tip nor mutate refs.
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).
A crafted receive-pack body can carry a branch create/update whose new_id is not stored on the server and no packfile. Import finalization then unwrapped the missing commit in traverses_tree_and_update_filepath, panicking the handler instead of reporting an ng status. For pack-less requests, every surviving non-deletion command must target an object the server already stores (real clients send nothing at all when everything is up to date), so such commands are now rejected with an explicit error before finalization. Import dispatch also applies only branch commands that survived validation, so a rejected command can neither be selected as the attach source nor mutate refs in the attach transaction. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83dc33cfba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.
For a pack-less import request whose non-deletion target is absent, the new validation marks the command ng, but this condition still finalizes every import push because !is_monorepo is true. Fresh evidence in the final tree is that ImportRepo::traverses_tree_and_update_filepath selects the failed command without checking its status and unwraps the missing commit, so the request panics instead of returning the intended per-ref ng report; require successful branch work for import finalization too, or make that traversal ignore failed commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
| if cmd.ref_type == RefTypeEnum::Branch && cmd.command_type != CommandType::Delete { | ||
| self.apply_cl_mega_ref_for_push_command(cmd, Some(&txn)) | ||
| .await?; |
There was a problem hiding this comment.
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 ng and has_branch_work therefore permits finalization. This loop nevertheless processes both non-delete commands because it ignores status; resolving the failed command's missing commit aborts and rolls back finalization, so the valid command does not receive the independent result implied by the validation. Filter this transaction to commands whose status is still ok.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
Finalization consumers still assumed every command survived: - The finalize gate exempted import repos, so a pack-less request whose only branch command failed target validation still ran traverses_tree_and_update_filepath, which selected the failed command without checking status and unwrapped its missing commit. Finalize now requires surviving branch work for both repo types. - Monorepo CL mega_refs persistence processed ng commands alongside ok ones; resolving the failed command's missing commit aborted the transaction and lost valid sibling updates. The transaction now filters to ok commands. - Import traversal selected the branch tip without checking status and could unwrap a rejected command's absent commit. - Monorepo tip metadata (base_branch/from_hash/to_hash) was captured at handler construction, before validation, so finalize events could carry a rejected command's hash. Tip metadata now lives behind a mutex and is re-derived from surviving commands in sync_commands_after_unpack. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb3a7cb4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for cmd in deletions { | ||
| git_db | ||
| .remove_ref_in_txn(repo_id, &cmd.ref_name, &txn) | ||
| .await?; |
There was a problem hiding this comment.
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 cmd.old_id. The deletion is therefore reported successful even if the branch has since advanced, allowing a stale push or --force-with-lease operation to delete another user's newer ref; make the deletion conditional on the advertised old object ID within the transaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
| let has_branch_work = commands | ||
| .iter() | ||
| .any(|c| c.ref_type == RefTypeEnum::Branch && c.status == "ok"); | ||
| if !unpack_failed && has_branch_work { |
There was a problem hiding this comment.
For a lightweight tag-only push to a monorepo, MonoRepo::update_refs routes the tag through apply_cl_mega_ref_for_push_command, which ignores cmd.ref_name and writes refs/cl/<link> instead. This new has_branch_work condition then skips finalization and leaves the tag command marked ok, so the client sees a successful push even though the requested refs/tags/* ref is never created; either reject monorepo tag pushes or persist the actual tag ref before reporting success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — valid, and the archaeology is worth spelling out: monorepo receive-pack has never actually persisted pushed tags. update_refs routed every command through apply_cl_mega_ref_for_push_command, which ignores cmd.ref_name and writes refs/cl/; before this PR every tag-only push then crashed in the finalize event (empty to_hash), so tags were loudly never created. My finalize gate turned that into a silent ok, which is worse.
Fixed in f5b0d42 by rejecting tag commands on the monorepo path up front ("tag pushes are not supported on monorepo; manage tags through the tag API"). Monorepo tags are fully supported via the tag API (mega_tag rows plus refs/tags/* mega_refs), so that remains the working path; implementing receive-pack tag ingestion properly means handling annotated-tag object storage and parsing, which is feature work beyond this PR rather than something to bolt onto update_refs.
Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean).
| 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.
When a pack-less import tag create/update points to an annotated-tag object already stored in the repository, this calls check_commit_exist, whose import implementation only queries the commit table. The valid tag object is consequently marked missing and the ref update is rejected; restrict this commit-only check to branch commands or perform an object-type-agnostic existence check for tags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
Three review findings on the deletion and tag paths: - Monorepo receive-pack never persisted pushed tags: update_refs routed every command through CL-ref materialization, writing refs/cl/<link> instead of the requested refs/tags/* ref. With finalization now gated, such pushes reported ok without creating anything; before the gate they crashed in the finalize event. Reject tag commands up front with a pointer to the tag API (which fully manages monorepo tags) until receive-pack support is implemented. - Import branch deletions removed refs by name only, ignoring the advertised old id. A branch advanced by a concurrent push after ref discovery could be deleted by a stale push. Deletions are now compare-and-delete within the transaction via a new GitDbStorage::get_ref_by_name_in_txn. - The pack-less target validation checked commits for all ref types, but an annotated tag's id is not a commit; restrict the check to branch commands. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5b0d42b73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let result = import_refs::Entity::find() | ||
| .filter(import_refs::Column::RepoId.eq(repo_id)) | ||
| .filter(import_refs::Column::RefName.eq(ref_name)) | ||
| .one(txn) |
There was a problem hiding this comment.
On the configured PostgreSQL backend, this ordinary SELECT does not lock the ref row, so another push can update the branch after this check but before remove_ref_in_txn; that later delete filters only by repository and name and can still remove the newer version. Fresh evidence after the prior deletion-race report is that the new implementation separates the old-ID comparison from the delete without FOR UPDATE or an old_id predicate. Perform a conditional delete including ref_git_id = cmd.old_id and verify one affected row, or lock the selected row before comparing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the check-then-delete was not atomic: on PostgreSQL the plain SELECT takes no lock, so a concurrent push could move the ref between compare and delete and the newer tip would still be removed.
Fixed in 374d7ac by replacing the read-then-delete with a single conditional delete (GitDbStorage::remove_ref_if_unchanged) that filters on ref_git_id = advertised old id inside the same transaction and reports whether a row was removed; a miss aborts with "ref moved since advertisement" instead of deleting anything. The getter added for the previous attempt was removed again since this needs no separate read.
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 -- -D warnings: clean).
| if cmd.ref_type != RefTypeEnum::Branch { | ||
| continue; | ||
| } | ||
| for &cmd in &branch_cmds { |
There was a problem hiding this comment.
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 Delete arm removes the ref solely by name, bypassing the old-ID validation used by the deletion-only helper. If that deleted branch advanced after advertisement, the mixed push therefore removes the newer tip while reporting success. Validate every deletion against cmd.old_id within the same transaction, including mixed command sets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
| return pack_protocol | ||
| .git_receive_pack_stream(state, commands, None) |
There was a problem hiding this comment.
On the new HTTP pack-less path, an import-repository tag deletion now reaches ImportRepo::update_refs, whose delete arm calls remove_ref using only the ref name. If the tag is moved or recreated after advertisement, the stale request still deletes the newer tag instead of returning ng; make tag deletion conditional on the advertised old_id before reporting success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — valid: tag deletions via update_refs used name-only removal, so a stale request could delete a newer tag. The pack-less path newly made this reachable without any pack.
Fixed in 374d7ac: the Delete arm now uses GitDbStorage::remove_ref_if_unchanged keyed on the advertised old id; when nothing matches it fails this command with "tag moved since advertisement", which surfaces as an ng line in the report while other commands are unaffected. (The Create arm keeps plain save semantics — recreating an existing tag is an update in git terms and clients send Update or force forms for that; flagging if you 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).
The old-id comparison for import deletions was a plain select followed by a delete filtered only by repo and name, which is not atomic on PostgreSQL: a concurrent push could move the ref between check and delete, and the newer tip would still be removed. The attach transaction's Delete arm and tag deletions via update_refs had the same name-only hazard. Deletions now go through GitDbStorage::remove_ref_if_unchanged, a single conditional delete keyed on the advertised old id that reports whether a row was removed; a miss aborts the transaction (or fails the tag command) with a 'moved since advertisement' error instead of deleting the newer ref. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374d7ac360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if command.status == "ok" | ||
| && command.ref_type == RefTypeEnum::Branch | ||
| && command.command_type != CommandType::Delete |
There was a problem hiding this comment.
When an HTTP or SSH pack-less import push creates or updates a tag to an arbitrary nonexistent object ID, this branch-only validation exempts the command and ImportRepo::update_refs immediately persists it with status ok. That leaves a dangling advertised tag whose object cannot be served during a subsequent fetch; annotated tags need an object-type-agnostic existence check rather than skipping validation for every tag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the tag exemption traded one hole for another: arbitrary nonexistent ids could be persisted as advertised dangling refs.
Fixed in e8fb6dc properly this time: import repos store annotated tag objects in the git_tag table during unpack, so validation is now type-aware instead of exempting tags. Branch commands check commits (check_commit_exist), tag commands check stored tag objects via a new RepoHandler::check_tag_exist hook backed by GitDbStorage::get_tag_by_hash. The hook defaults to false so repo types that do not track tag objects reject unverifiable targets rather than accepting them blind; monorepo tags never reach it since they are rejected up front.
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 and -p jupiter --all-targets -D warnings: clean).
| for cmd in &cmds { | ||
| if cmd.ref_type == RefTypeEnum::Branch { | ||
| if cmd.ref_type == RefTypeEnum::Branch | ||
| && cmd.status == "ok" | ||
| && cmd.command_type != CommandType::Delete | ||
| { | ||
| self.apply_cl_mega_ref_for_push_command(cmd, Some(&txn)) |
There was a problem hiding this comment.
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 refs/cl/<link> ref, so the later command overwrites the earlier one while finalization metadata still comes from the first surviving command. A normal multi-ref push such as creating two branches from advertised commits therefore acknowledges two updates even though only one target is represented; enforce the documented single-commit restriction before persisting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
Two gaps in the pack-less validation and monorepo guard: - The previous commit exempted tags from existence validation because their id may be an annotated tag object, but that allowed arbitrary nonexistent ids to be persisted as advertised dangling refs. Import repos store annotated tag objects in the git_tag table during unpack, so tag commands now validate against it via a new check_tag_exist hook (default false: repo types that do not track tag objects reject unverifiable targets rather than accepting them blind). - Monorepo pushes with two surviving branch commands reported both ok while apply_cl_mega_ref_for_push_command materialized both through the same refs/cl/<link> ref, silently keeping only the last. Enforce the documented single-commit-per-push restriction up front (packed pushes already hit it in check_entry). Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8fb6dcad8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if surviving_branch_update { | ||
| command.failed( | ||
| "monorepo pushes support at most one branch update".to_string(), | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
For a pack-less monorepo request ordered as [missing-target update, valid existing-target update], the first command sets surviving_branch_update here, so the valid second command is rejected as an excess update. The later existence check then marks the first command ng but skips the already-rejected second command, leaving no update even though only one command actually survived validation. Perform target validation before enforcing the single-branch limit, or count only commands that remain ok afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — ordering bug: the cap counted the missing-target command first, so the valid update was rejected as excess and then validation ng'"'"'d the invalid one, stranding both.
Fixed in 5258883 by reordering: target validation now runs first, and the single-branch-update cap counts only commands that remain ok afterward. For [missing-target, valid] the valid update survives and finalizes; for two valid updates the second is still rejected as an excess.
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 exists = match command.ref_type { | ||
| RefTypeEnum::Tag => repo_handler.check_tag_exist(&command.new_id).await, | ||
| _ => repo_handler.check_commit_exist(&command.new_id).await, |
There was a problem hiding this comment.
For a pack-less import push creating a lightweight tag to an existing commit, tree, or blob, this selects check_tag_exist, whose newly added implementation only queries the git_tag table for annotated-tag objects. The valid lightweight tag is therefore reported as having a missing target even though its target object is already stored. Tag validation needs to check all supported Git object tables rather than only annotated tags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — valid: lightweight tags can reference any object type, so checking only the annotated-tag table falsely rejected valid targets.
Fixed in 5258883 by generalizing the hook to RepoHandler::check_object_exist with union semantics over stored tag objects, commits, trees, and blobs (all four tables exist for import repos). Branch commands still require a commit, since a branch pointing at a tree or blob is not servable as a ref target.
Verified in Docker (cargo test -p ceres --lib transport: 10 passed, 0 failed; cargo clippy -p ceres --all-targets --all-features -- -D warnings: clean).
… targets - The single-branch-update cap counted commands before target validation, so [missing-target update, valid update] rejected the valid command as an excess while the invalid one was then marked ng, leaving no update. Validation now runs first and the cap counts only surviving commands. - Tag existence checking only consulted the annotated-tag table, falsely rejecting lightweight tags whose commit, tree, or blob targets are stored. check_tag_exist becomes RepoHandler::check_object_exist with union semantics over tag, commit, tree, and blob objects; branches keep requiring commits. Signed-off-by: Tyagiquamar <Tyagiquamar@users.noreply.github.com> Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52588832b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let exists = match command.ref_type { | ||
| RefTypeEnum::Branch => repo_handler.check_commit_exist(&command.new_id).await, | ||
| _ => repo_handler.check_object_exist(&command.new_id).await, | ||
| }; |
There was a problem hiding this comment.
When an import-repository ref advances after advertisement and a pack-less update targets an object already stored, this validation accepts the command based only on new_id. Branch finalization then calls update_ref_in_txn, while the tag path calls update_ref; both update by repository and ref name without comparing old_id, so a stale normal push or --force-with-lease can overwrite another user's newer ref. Make these updates conditional on the current ref_git_id matching the advertised old ID and verify that one row changed.
Useful? React with 👍 / 👎.
| if let Err(e) = repo_handler.update_refs(command).await { | ||
| command.failed(e.to_string()); |
There was a problem hiding this comment.
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 remove_ref_if_unchanged failure path for nondefault deletions. Defer tag persistence until branch validation succeeds, or include all ref mutations in the same transaction.
Useful? React with 👍 / 👎.
2 participants
Problem
git_receive_packlocated the start of the packfile by scanning each raw HTTP chunk for the byte sequencePACK(search_subsequence(&chunk, b"PACK")). That breaks in three ways:git push origin --delete branch, or a no-op ref update — never contains thePACKmagic. The read loop drains the body without ever matching,report_statusstays empty, and the server returns an empty 200 while the pushed commands are never processed. I verified this empirically: capturing the request body of a realgit push --delete mainshows it consists only of pkt-line commands followed by the flush packet, with noPACKmagic anywhere (178 bytes,indexOf('PACK') == -1).…PAC|K…); searching within individual chunks then never finds it, producing the same silent empty-report failure.PACK(e.g.refs/heads/JDK-PACK) matches mid-command, truncating the command list inside a pkt-line and feeding binary garbage to the unpacker.The same scan is used by the SSH receive-pack path.
Fix
Frame the body by protocol structure instead of magic-byte scanning:
split_commands_and_packhelper walks pkt-line length headers from the accumulated buffer until the terminating flush packet (0000) and splits off the command section; everything after the flush is pack data. Malformed length headers now returnProtocolError::InvalidInput(HTTP 400) instead of panicking.git_receive_pack_streamnow takesOption<PackByteStream>and skips unpack entirely, so ref updates and the status report still run. Previously this path could not be reached at all.search_subsequence.Testing
mono/src/git_protocol/http.rscover: normal framing, flush marker straddling chunk boundaries,PACKappearing inside a ref name (must stay command data), and invalid/short length headers being rejected.cargo test -p mono --lib git_protocol— 4 passedcargo test -p ceres --lib transport— 10 passedcargo clippy -p mono -p ceres --all-targets --all-features -- -D warnings— cleancargo +nightly fmt --checkclean on all touched filescargo test -p ceres --lib: 160 passed; 9 failures inapplication::code_edit/application::build_triggerreproduce identically on unmodifiedmainin my environment (unrelated DB-dependent tests)Signed-off-by: Tyagiquamar Tyagiquamar@users.noreply.github.com