Skip to content

Fix session recording truncation #60891

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.

Sign up for GitHub

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

Merged
Joerger merged 7 commits into from
Nov 3, 2025
Merged

Fix session recording truncation #60891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7cec059
Cleanup empty uploads rather than completing them.
Joerger Oct 31, 2025
64e4b59
Add regression test.
Joerger Oct 31, 2025
af8087e
Update lib/events/filesessions/filestream_test.go
Joerger Oct 31, 2025
aa973fe
Remove test for uploading an empty reservation upload.
Joerger Oct 31, 2025
9177f92
Cleanup after FS lock.
Joerger Oct 31, 2025
fa65c63
Revert "Cleanup after FS lock."
Joerger Nov 3, 2025
e9ce236
Merge branch 'master' into joerger/fix-audit-disk-truncated
Joerger Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions lib/events/filesessions/filestream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ func (h *Handler) CompleteUpload(ctx context.Context, upload events.StreamUpload
return trace.Wrap(err)
}

// If there are no parts to complete, move to cleanup
if len(parts) == 0 {
return h.cleanupUpload(ctx, upload)
}

uploadPath := h.recordingPath(upload.SessionID)

// Prevent other processes from accessing this file until the write is completed
Expand Down Expand Up @@ -251,6 +256,22 @@ Loop:
return nil
}

func (h *Handler) cleanupUpload(ctx context.Context, upload events.StreamUpload) error {
uploadKey := h.recordingPath(upload.SessionID)
log := h.logger.With(
"upload", upload.ID,
"session", upload.SessionID,
"key", uploadKey,
)
log.DebugContext(ctx, "Aborting upload")
if err := os.RemoveAll(h.uploadRootPath(upload)); err != nil {
h.logger.ErrorContext(ctx, "Failed to remove upload", "upload_id", upload.ID)
}

log.InfoContext(ctx, "Aborted upload")
return nil
}

// ListParts lists upload parts
func (h *Handler) ListParts(ctx context.Context, upload events.StreamUpload) ([]events.StreamPart, error) {
var parts []events.StreamPart
Expand Down
53 changes: 45 additions & 8 deletions lib/events/filesessions/filestream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,6 @@ func TestCompleteUpload(t *testing.T) {
createPart(t, handler, upload, int64(5), []byte("withreservation"))
},
},
{
desc: "OnlyReservation",
expectedContent: []byte{},
partsFunc: func(t *testing.T, handler *Handler, upload *events.StreamUpload) {
createPart(t, handler, upload, int64(1), []byte{})
createPart(t, handler, upload, int64(2), []byte{})
},
},
Comment on lines -131 to -138
Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rosstimothy FYI I had to remove this test as it was basically ensuring that we do upload empty uploads if we find an empty upload instead of discarding it.

} {
t.Run(test.desc, func(t *testing.T) {
handler, err := NewHandler(Config{
Expand Down Expand Up @@ -169,3 +161,48 @@ func TestCompleteUpload(t *testing.T) {
})
}
}

func TestCleanupEmptyUpload(t *testing.T) {
ctx := t.Context()

handler, err := NewHandler(Config{
Directory: t.TempDir(),
OpenFile: os.OpenFile,
})
require.NoError(t, err)

sessionID := session.NewID()

// Create a completed upload.
upload, err := handler.CreateUpload(ctx, sessionID)
require.NoError(t, err)

err = handler.ReserveUploadPart(ctx, *upload, 1)
require.NoError(t, err)

content := []byte("hello world")
part, err := handler.UploadPart(ctx, *upload, 1, bytes.NewReader(content))
require.NoError(t, err)

err = handler.CompleteUpload(ctx, *upload, []events.StreamPart{*part})
require.NoError(t, err)

// Create an empty upload with the same session ID and try to complete it.
emptyUpload, err := handler.CreateUpload(ctx, sessionID)
require.NoError(t, err)

err = handler.CompleteUpload(ctx, *emptyUpload, []events.StreamPart{})
require.NoError(t, err)

// The empty upload should be cleaned up without impacting the original completed upload.
uploadPath := handler.recordingPath(upload.SessionID)
f, err := os.Open(uploadPath)
require.NoError(t, err)

gotContent, err := io.ReadAll(f)
require.NoError(t, err)
require.Equal(t, content, gotContent)
Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take a closer look at this fix since test is failing in CI.

        	Error:      	Not equal: 
        	            	expected: []byte{0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x20, 0x77, 0x6f, 0x72, 0x6c, 0x64}
        	            	actual  : []byte{}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,2 @@
        	            	-([]uint8) (len=11) {
        	            	- 00000000  68 65 6c 6c 6f 20 77 6f  72 6c 64                 |hello world|
        	            	+([]uint8) {
        	            	 }
        	Test:       	TestCleanupEmptyUpload

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was caused by 9177f92, reverted. Sorry for the haphazard change, in addition to being done wrong, I don't think it's necessary.


require.NoDirExists(t, handler.uploadRootPath(*upload))
require.NoDirExists(t, handler.uploadRootPath(*emptyUpload))
}
Loading