bug: client disconnect (ctrl+c) on /push leaves push record dangling in RECEIVED/PENDING

[coopernetes]

Problem

When a git client cancels a push to the store-and-forward path (/push/) mid-flight — e.g. by pressing ctrl+c — the push record is left in RECEIVED or PENDING status and is never cleaned up. The push is never forwarded upstream.

What happens today

1. PushStorePersistenceHook.preReceive creates the record with status RECEIVED.
2. The hook chain runs validation and transitions to PENDING (blocked) or REJECTED.
3. If the client disconnects at any point before the ForwardingPostReceiveHook runs, the JGit session ends without the post-receive hook firing.
4. The record stays in RECEIVED or PENDING indefinitely — there is no sweeper, no TTL, and no mechanism to detect the disconnect and mark the record as cancelled or errored.

HeartbeatSender detects the socket closure (it shuts itself down on any IOException when writing to the sideband) but currently has no way to signal that the push is dead.

Impact

• Dangling records accumulate in the push store.
• Reviewers may see stale PENDING entries in the dashboard that can never be actioned — approving them will not cause anything to happen because the originating git session is long gone.
• The approval timeout on ApprovalPreReceiveHook (default 30 min) handles the case where the client is still connected but no approval arrives; it does not help here because the JGit thread itself has already exited.

Possible approaches

point 1 — HeartbeatSender signals disconnect

Wire a disconnect callback through PushContext so that when HeartbeatSender catches a socket IOException, it calls back into PushStorePersistenceHook (or a dedicated handler) to mark the in-progress record as CANCELED or ERROR.

Pros: real-time, no polling. Cons: only works while the heartbeat thread is alive; a very fast disconnect before the first heartbeat tick may not be caught.

point 2 — Stale-record sweeper job

A background thread (or scheduled task) that periodically queries for push records stuck in RECEIVED or PENDING for longer than a configurable TTL (e.g. 1 hour) and transitions them to ERROR with a reason like "push session abandoned".

Pros: catches all cases including very fast disconnects and server restarts. Cons: lag before the record is marked stale; need to be careful not to sweep records that are legitimately waiting for human approval.

point 3 — Combination

Use point 1 for the fast/clean case (client sends TCP FIN) and point 2 as a safety net for cases where the disconnect was silent or the server restarted.

Notes

• A record in PENDING that is awaiting human approval is legitimate and should not be swept prematurely. The sweeper would need to distinguish between records that have an active JGit session behind them (cannot easily be determined after restart) and records where the session is gone. A practical heuristic: if a record has been in RECEIVED status (pre-validation) for more than a few minutes, it is almost certainly dead; PENDING records need a longer TTL.
• The cancel API endpoint already exists and sets status to CANCELED — the sweeper or disconnect handler can reuse this path.

[coopernetes]

After review, the scope for this fix is narrowed to the heartbeat-based disconnect detection only.

What we're doing:

• Add an onDisconnect callback to HeartbeatSender. When sendDot() catches a socket IOException, the callback fires once, marking the push record CANCELED via the existing PushStore.cancel() path.
• The callback resolves the record ID from PushContext: uses validationRecordId if the push reached the validation result stage (PENDING), otherwise falls back to pushId (RECEIVED).

What we're NOT doing:

• No stale-record sweeper. A TTL-based sweeper cannot reliably distinguish an abandoned RECEIVED record from a PENDING record legitimately sitting in the approval queue. The complexity and risk of false cancellations outweigh the benefit.
• No transparent proxy changes. From the proxy's perspective, FORWARDED means the proxy forwarded the request upstream — what the upstream does with it is outside the proxy's responsibility.

Known gap: A client that disconnects faster than the heartbeat interval (default 10s) will not be caught. This is acceptable — the record stays RECEIVED briefly and is cleaned up if the JGit session eventually closes normally. The heartbeat covers the real problem case: a client that connects, waits through validation/approval, then drops.