TT-7621 WIP: undo a take whose upload lands after it was discarded - #531
Draft
nabalone wants to merge 3 commits into
Draft
TT-7621 WIP: undo a take whose upload lands after it was discarded#531nabalone wants to merge 3 commits into
nabalone wants to merge 3 commits into
Conversation
nabalone
force-pushed
the
TT-7621_pbt-segment-selection-and-recorder-state
branch
from
August 24, 2026 21:56
5bef41e to
3f3ee57
Compare
nabalone
force-pushed
the
TT-7621_pbt-discard-take-mid-upload
branch
from
August 24, 2026 21:57
0d56f45 to
59682ff
Compare
Base automatically changed from
TT-7621_pbt-segment-selection-and-recorder-state
to
develop
August 24, 2026 22:33
INCOMPLETE - the repro test is still red. Committed so the analysis is not lost. Clearing a take while its upload is in flight brings the take back: the upload finishes, the mediafile reaches rowData, and the step shows a take the user deleted with Record disabled and the segment counted as recorded. What is here: - uploadInFlightRef, so "is an upload still in flight" can actually be answered. savingRecording cannot answer it: by the time the delete icon is clickable it is already false, which is why an earlier attempt at this never triggered. - afterUploadCb returns early for a discarded take instead of forcing phase 'recorded' and marking the segment optimistically complete. - an effect that removes the mediafile once it appears in rowData, since at afterUploadCb time there is nothing to address yet. Why it is still red: the delete icon is still present at the end of the test, so something puts the step back into phase 'recorded'. The most likely candidate is the recording-pass navigation effect's completed branch - once the take is in rowData, completedIndices contains the current index and that branch sets 'recorded' - racing the cleanup effect above. Confirming it needs the step's phase to be observable; adding data-phase to the container made short work of the equivalent question on the Record-during-playback investigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things, both from chasing the discard-mid-upload defect. Harness: with putDelayMs the fake server added the mediafile when the PUT started, not when it completed, so the take appeared in rowData while the upload was still in flight. A test about what happens *during* an upload was really testing what happens after one - and it is what made the first attempt at this fix look like it was not firing. Storage is now timed from the PUT completing. Worth taking regardless of the rest of this branch. Step: data-phase, data-allow-record, data-unit-index and (temporarily) data-discard-pending on the container. The listen/record state machine drives most of this step's behaviour and was invisible from outside, which made a wrong Record state guesswork to diagnose; two investigations tonight collapsed to one run each once it could be read. Still red. With the timing corrected the discard flag now survives to afterUploadCb as intended, but the step still ends in phase 'recorded' with the take shown, so the cleanup effect's removeRecord is not taking effect - either it is losing a race with the navigation effect's completed branch, or the removal itself is not landing. Next step is to log around the removal rather than infer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Still red. Third and last attempt tonight; recorded so tomorrow starts from what is known rather than from scratch. The marker is now the unit index that was discarded rather than a boolean, and neither afterUploadCb nor the cleanup effect clears it on the other's behalf: whichever of them runs first, the other still has to act. That was the flaw in the previous attempt - sampling showed the cleanup effect deleting the arriving mediafile and consuming the flag, and afterUploadCb then running with nothing set and taking the ordinary success path, so the segment ended up 'recorded' with the take shown and optimistically complete. It also clears the marker when a new take starts, so a later recording on the same segment is not mistaken for the discarded one. What sampling established, for whoever picks this up: - the discard marker is set (uploadInFlightRef answers correctly) - the mediafile does arrive and the cleanup effect does delete it - rowData never grows, the row appears and goes between two 50ms samples - the step still ends in phase 'recorded' with the delete icon showing So what remains is not the deletion but the phase: something re-asserts 'recorded' after the cleanup. The navigation effect's completed branch is the prime suspect (it sets 'recorded' whenever completedIndices contains the current index) together with the optimistic-completion set, which afterUploadCb populates and which is not rowData-backed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nabalone
force-pushed
the
TT-7621_pbt-discard-take-mid-upload
branch
from
August 25, 2026 12:40
59682ff to
743eded
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP — do not merge. The repro test is still red. Pushed so the analysis is not lost.
The fault
Clearing a take while its upload is in flight brings the take back: the upload finishes, the mediafile reaches rowData, and the step shows a take the user deleted — Record disabled, segment counted as recorded, audio they rejected stored.
Worth taking regardless of the rest: a harness timing bug
With
putDelayMs, the fake server added the mediafile when the PUT started rather than when it completed, so the take appeared in rowData while the upload was still in flight. A test about what happens during an upload was really testing what happens after one — and it is what made my first attempt at this fix look like it wasn't firing at all. Storage is now timed from the PUT completing.Also here: the step's state machine is now observable
data-phase,data-allow-record,data-unit-indexon the step container (plus a temporarydata-discard-pending). The listen/record state machine drives most of this step's behaviour and was invisible from outside. Two investigations tonight collapsed from repeated guessing to one run each once it could be read — including the one that stopped me shipping a bad fix in #529.The fix attempt
uploadInFlightRef, so "is an upload still in flight" can be answered.savingRecordingcannot: by the time the delete icon is clickable it is already false.afterUploadCbreturns early for a discarded take instead of forcing phase'recorded'and marking the segment optimistically complete.afterUploadCbtime there is nothing to address yet.Where it stands
With the timing corrected, the discard flag now survives to
afterUploadCbas intended — confirmed by samplingdata-discard-pending. But the step still ends in phase'recorded'with the take shown, so the cleanup effect'sremoveRecordis not taking effect: either it loses a race with the navigation effect's completed branch (which sets'recorded'as soon ascompletedIndicescontains the index), or the removal itself is not landing.Next step is to log around the removal rather than infer.
🤖 Generated with Claude Code