handlePush re-acks a repeated chunk without re-appending it (cmd/sessiond/files.go:415, "the ack was lost, not the data"), which is what lets one dropped response survive on every chunk but the last. On success the final chunk's handler runs defer t.abandonPush(c.Xfer, x) (line 452), deleting the transfer from t.pushes the moment it finishes extracting.
If the client never sees that ack and resends the identical final chunk, the lookup finds nothing, and because a nonzero Seq fails the "transfer hasn't started" guard (line 396), it returns push chunk %d arrived for a transfer that has not started for a transfer that in fact already succeeded.
A client treating that as fatal either reports a false failure for a push that actually landed, or restarts the whole multi-chunk transfer under the same id and re-extracts the archive.
Reproduced with a unit test driving handlePush directly: push a two-chunk transfer to completion, then do exactly what a client recovering from a lost ack normally does, resend the last chunk unchanged, same as TestPushReAcksARepeatedChunk does for an intermediate one.
BUG CONFIRMED: retry of the completed final chunk (Seq 8, already extracted) returned an error instead of the documented re-ack: push chunk 8 arrived for a transfer that has not started
go test ./cmd/sessiond/... -race -count=1 is otherwise green, so this isn't colliding with anything else in the suite.
Two ways I can see to close it, and they cut against something open()'s own doc comment currently promises ("tests assert every exit path takes its entry with it", pinned by TestPushRoundTrip's open() == 0 check right after a successful round trip):
- keep the entry in
t.pushes after a successful final chunk instead of deleting it, clearing only the staging file, and let the idle sweep reclaim it later. Reuses the existing re-ack path as is, but a completed transfer would then count in open() until swept, which isn't what the comment and test say today.
- a small separate record of just-completed transfers (id, final seq) that
handlePush checks before the "hasn't started" error, so open() keeps meaning "in-flight" exactly as it does now.
Happy to send a PR for either once you have a preference. The mirror path in handlePull doesn't have this problem: reading is stateless, so retrying the last read chunk is just another read.
handlePushre-acks a repeated chunk without re-appending it (cmd/sessiond/files.go:415, "the ack was lost, not the data"), which is what lets one dropped response survive on every chunk but the last. On success the final chunk's handler runsdefer t.abandonPush(c.Xfer, x)(line 452), deleting the transfer fromt.pushesthe moment it finishes extracting.If the client never sees that ack and resends the identical final chunk, the lookup finds nothing, and because a nonzero
Seqfails the "transfer hasn't started" guard (line 396), it returnspush chunk %d arrived for a transfer that has not startedfor a transfer that in fact already succeeded.A client treating that as fatal either reports a false failure for a push that actually landed, or restarts the whole multi-chunk transfer under the same id and re-extracts the archive.
Reproduced with a unit test driving
handlePushdirectly: push a two-chunk transfer to completion, then do exactly what a client recovering from a lost ack normally does, resend the last chunk unchanged, same asTestPushReAcksARepeatedChunkdoes for an intermediate one.go test ./cmd/sessiond/... -race -count=1is otherwise green, so this isn't colliding with anything else in the suite.Two ways I can see to close it, and they cut against something
open()'s own doc comment currently promises ("tests assert every exit path takes its entry with it", pinned byTestPushRoundTrip'sopen() == 0check right after a successful round trip):t.pushesafter a successful final chunk instead of deleting it, clearing only the staging file, and let the idlesweepreclaim it later. Reuses the existing re-ack path as is, but a completed transfer would then count inopen()until swept, which isn't what the comment and test say today.handlePushchecks before the "hasn't started" error, soopen()keeps meaning "in-flight" exactly as it does now.Happy to send a PR for either once you have a preference. The mirror path in
handlePulldoesn't have this problem: reading is stateless, so retrying the last read chunk is just another read.