From b8f57ed5f649cf9ef032dd866934f305c34b4e53 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:43:30 +0200 Subject: [PATCH 01/14] feat(stream): drop missions the producer discards The addon enforces its save limits, such as the minimum mission time, by not saving a recording. A streamed recording already lives on the server, though, and was saved as partial once its producer went away, so missions the addon chose not to keep showed up anyway. Producers now end such a mission with discard_mission instead of end_mission. The server removes the streaming row and its files, along with partial segments earlier attempts of the same mission id left behind, releases the session slot and acknowledges it like end_mission. The mission id is remembered as finished, so a producer whose acknowledgement was lost gets resume_completed. start_mission is now acknowledged with protocol version 3, which tells the extension that discard_mission is understood; older servers never receive it. Bumps the extension module for the protocol constants. --- docs/streaming-architecture.md | 24 ++- go.mod | 2 +- go.sum | 6 +- internal/server/handler_stream.go | 68 +++++++- .../server/handler_stream_discard_test.go | 152 ++++++++++++++++++ internal/server/handler_stream_resume_test.go | 4 +- 6 files changed, 245 insertions(+), 11 deletions(-) create mode 100644 internal/server/handler_stream_discard_test.go diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index 66591b58..1c413a89 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -73,7 +73,8 @@ the next section. - `start_mission` carries a `missionId` (a random UUID per recording) and a `producerId` (a random UUID per extension process). Every envelope carries a `seq`, starting with 1 for `start_mission`. -- The server acknowledges `start_mission` with `protocolVersion: 2`, and then +- The server acknowledges `start_mission` with `protocolVersion: 3` (resume + plus `discard_mission`, see below), and then acknowledges received envelopes cumulatively (`{"type":"ack","seq":N}`) every `streaming.ackInterval` and during bursts. The extension holds every envelope in memory until it is acknowledged. @@ -147,6 +148,27 @@ still replaces the earlier partial segments when nothing is missing. Missions whose save was never confirmed are recovered the same way, with their own `end_mission`. +### Discarding a mission + +The addon enforces its save limits, such as `OCAP_settings_minMissionTime`, by +not saving a recording. A streamed recording already lives on the server, +which would save it as `partial` once its producer is gone. So instead of +`end_mission` the producer sends a numbered `discard_mission` (no payload): + +- The server removes the recording's `streaming` row and files, along with the + partial segments earlier attempts of the same mission id left behind, and + acknowledges it like `end_mission` (`{"type":"ack","for":"discard_mission"}`). +- The mission id is remembered as finished, so a producer whose acknowledgement + was lost gets `resume_completed`. +- An unconfirmed discard stays in the extension's spool. Recovery sends only the + `discard_mission` again: a server that no longer holds the mission + (`resume_unknown`) has nothing left to drop, and the mission is not resent. + +The extension sends `discard_mission` only to servers that acknowledged +`start_mission` with `protocolVersion: 3` or later. With an older server it +closes the connection instead, and that server saves the recording as +`partial`. + ### Legacy producers For producers without `protocolVersion: 2`, disconnecting without `end_mission`, diff --git a/go.mod b/go.mod index c8f218cf..654f8493 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261019201552-0ebd39eb0267 + github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index cdc01df3..d6d25466 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261019173050-9ef6f2444ee8 h1:FgLuVMC2vSqwhfELiMGXuEJaN36qNY9N4m/0e5ioqQU= -github.com/OCAP2/extension/v5 v5.0.2-0.20261019173050-9ef6f2444ee8/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= -github.com/OCAP2/extension/v5 v5.0.2-0.20261019201552-0ebd39eb0267 h1:14dPRuYh/Cr+POYlnJWMyNo9PVnRT7K31MBLJw4TkNU= -github.com/OCAP2/extension/v5 v5.0.2-0.20261019201552-0ebd39eb0267/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e h1:jvLRAZDjLYx0LoOayo0b2z7/m+lbn5Jjl+B3nVeuyt8= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 12ccbd63..2998da35 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -175,7 +175,7 @@ func (h *Handler) streamLoop(ws *websocket.Conn) { ack := streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeStartMission} if started.resumable() { ack.Seq = started.lastSeq.Load() - ack.ProtocolVersion = streaming.ProtocolVersionResume + ack.ProtocolVersion = streaming.ProtocolVersionDiscard } writeJSON(ack) @@ -212,6 +212,19 @@ func (h *Handler) streamLoop(ws *websocket.Conn) { writeJSON(ack) return + case streaming.TypeDiscardMission: + if ss == nil { + slog.Info("stream: mission discarded without an active session") + writeJSON(streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission}) + return + } + if !h.discardStream(ss, conn, envelope) { + return // another connection took the mission over + } + current.Store(nil) + writeJSON(streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission, Seq: ss.lastSeq.Load()}) + return + default: // All other messages require an active session. if ss == nil { @@ -511,15 +524,64 @@ func (h *Handler) replaceStreamSegments(ctx context.Context, ss *streamSession) return } for _, seg := range segments { - if err := h.repoOperation.Delete(ctx, seg.ID); err != nil && !errors.Is(err, ErrNotFound) { + if err := h.removeStreamOperation(ctx, seg.ID, seg.Filename); err != nil { slog.Warn("stream: cannot remove earlier segment", "id", seg.ID, "error", err) continue } - removeRecordingArtifacts(h.setting.Data, seg.Filename) slog.Info("stream: replaced partial segment with complete recording", "missionId", ss.missionID, "removed", seg.Filename, "kept", filepath.Base(ss.outputDir)) } } +// discardStream drops the session for discard_mission: the producer decided +// the recording does not meet its save limits, for example the minimum +// mission duration. The streaming row and its files go, along with any +// partial segments an earlier attempt of the mission left behind. It reports +// false when conn no longer feeds the session. +func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope streaming.Envelope) bool { + ss.mu.Lock() + defer ss.mu.Unlock() + if ss.owner != conn || ss.done { + return false + } + ss.done = true + if envelope.Seq > ss.lastSeq.Load() { + ss.lastSeq.Store(envelope.Seq) + } + slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) + + ctx := context.Background() + if err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)); err != nil { + slog.Warn("stream: cannot remove discarded mission", "id", ss.opID, "error", err) + } + if ss.resumable() { + segments, err := h.repoOperation.StreamSegments(ctx, ss.missionID, ss.opID) + if err != nil { + slog.Warn("stream: cannot list earlier segments", "missionId", ss.missionID, "error", err) + } + for _, seg := range segments { + if err := h.removeStreamOperation(ctx, seg.ID, seg.Filename); err != nil { + slog.Warn("stream: cannot remove earlier segment", "id", seg.ID, "error", err) + } + } + // Remembered as finished, so a producer whose acknowledgement was + // lost learns from resume that the discard went through. + h.streamRegistry().remove(ss, true) + } + h.streamRegistry().release() + return true +} + +// removeStreamOperation deletes a streamed operation row and its files. +func (h *Handler) removeStreamOperation(ctx context.Context, id int64, filename string) error { + if id > 0 { + if err := h.repoOperation.Delete(ctx, id); err != nil && !errors.Is(err, ErrNotFound) { + return err + } + } + removeRecordingArtifacts(h.setting.Data, filename) + return nil +} + // streamEnded reports whether the database holds a saved recording of the // producer mission, which outlives the in-memory registry across restarts. func (h *Handler) streamEnded(missionID string) bool { diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go new file mode 100644 index 00000000..f0f0d39c --- /dev/null +++ b/internal/server/handler_stream_discard_test.go @@ -0,0 +1,152 @@ +package server + +import ( + "os" + "strings" + "testing" + "time" + + "github.com/OCAP2/extension/v5/pkg/streaming" + "github.com/gorilla/websocket" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingFiles lists what the data directory holds besides the database. +func (rh *resumeHarness) recordingFiles() []string { + rh.t.Helper() + entries, err := os.ReadDir(rh.dir) + require.NoError(rh.t, err) + var out []string + for _, e := range entries { + if !strings.HasPrefix(e.Name(), "test.db") { + out = append(out, e.Name()) + } + } + return out +} + +// requireClosed checks that the server closed the connection. +func requireClosed(t *testing.T, c *websocket.Conn) { + t.Helper() + c.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + if _, _, err := c.ReadMessage(); err != nil { + return + } + } +} + +func TestStreamDiscard_RemovesRecording(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Discard", "m-d", "p-1") + last := sendStates(t, c, 2, 1, 5) + rh.waitSeq("m-d", last) + require.Len(t, rh.ops(), 1, "a streaming row exists while the mission records") + + sendEnv(t, c, streaming.TypeDiscardMission, last+1, nil) + ack := readUntil(t, c, isAckFor(streaming.TypeDiscardMission)) + assert.Equal(t, last+1, ack.Seq, "the discard is acknowledged with everything before it") + requireClosed(t, c) + + assert.Empty(t, rh.ops()) + assert.Empty(t, rh.recordingFiles(), "no files are left behind") + reg := rh.h.streamRegistry() + assert.Nil(t, reg.get("m-d")) + assert.True(t, reg.isCompleted("m-d")) + assert.Zero(t, reg.activeCount(), "the session's slot is released") +} + +func TestStreamDiscard_LostAckResolvedByResume(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Lost Ack", "m-lost", "p-1") + sendEnv(t, c, streaming.TypeDiscardMission, 2, nil) + readUntil(t, c, isAckFor(streaming.TypeDiscardMission)) + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-lost", ProducerID: "p-1"}) + readUntil(t, c2, isType(streaming.ServerResumeCompleted)) + assert.Empty(t, rh.ops()) +} + +func TestStreamDiscard_ResumedSession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Resumed", "m-res", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-res", last) + c.Close() + + // The producer finishes an unconfirmed discard on a new connection. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-res", ProducerID: "p-1"}) + reply := readUntil(t, c2, isType(streaming.ServerResumeOK)) + sendEnv(t, c2, streaming.TypeDiscardMission, reply.Seq+1, nil) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + + assert.Empty(t, rh.ops()) + assert.Empty(t, rh.recordingFiles()) +} + +func TestStreamDiscard_RemovesEarlierSegments(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Segments", "m-seg", "p-1") + sendStates(t, c, 2, 1, 5) + rh.waitSeq("m-seg", 6) + c.Close() + // The producer stays away past the grace period: a partial segment is saved. + require.Eventually(t, func() bool { return rh.h.streamRegistry().get("m-seg") == nil }, 5*time.Second, 10*time.Millisecond) + require.Len(t, rh.opsByMission("m-seg"), 1) + + // It returns, resends the mission and then discards it. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-seg", ProducerID: "p-1"}) + readUntil(t, c2, isType(streaming.ServerResumeUnknown)) + startResumable(t, c2, "Segments", "m-seg", "p-1") + last := sendStates(t, c2, 2, 1, 8) + sendEnv(t, c2, streaming.TypeDiscardMission, last+1, nil) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + + assert.Empty(t, rh.ops(), "every segment of the discarded mission is removed") + assert.Empty(t, rh.recordingFiles()) +} + +func TestStreamDiscard_LeavesOtherMissionsAlone(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + other := rh.dial() + replayMission(t, other, "Kept", "m-kept", "p-2", 3) + + c := rh.dial() + startResumable(t, c, "Dropped", "m-drop", "p-1") + sendEnv(t, c, streaming.TypeDiscardMission, 2, nil) + readUntil(t, c, isAckFor(streaming.TypeDiscardMission)) + + ops := rh.ops() + require.Len(t, ops, 1) + assert.Equal(t, "Kept", ops[0].MissionName) +} + +func TestStreamDiscard_WithoutSession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + sendEnv(t, c, streaming.TypeDiscardMission, 0, nil) + readUntil(t, c, isAckFor(streaming.TypeDiscardMission)) + requireClosed(t, c) +} + +func TestStreamDiscard_LegacySession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + sendStartMission(t, c, "Legacy", "altis") + sendEnv(t, c, streaming.TypeSoldierState, 0, soldierState(1)) + sendEnv(t, c, streaming.TypeDiscardMission, 0, nil) + readUntil(t, c, isAckFor(streaming.TypeDiscardMission)) + requireClosed(t, c) + + assert.Empty(t, rh.ops(), "the disconnect that follows does not save it as partial") + assert.Empty(t, rh.recordingFiles()) + assert.Zero(t, rh.h.streamRegistry().activeCount()) +} diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index 6f71606b..aeecfa90 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -133,7 +133,7 @@ func TestStreamResume_StartAckAdvertisesResume(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() ack := startResumable(t, c, "Advertise", "m-1", "p-1") - assert.Equal(t, uint8(streaming.ProtocolVersionResume), ack.ProtocolVersion) + assert.Equal(t, uint8(streaming.ProtocolVersionDiscard), ack.ProtocolVersion) assert.Equal(t, uint64(1), ack.Seq) assert.NotNil(t, rh.h.streamRegistry().get("m-1")) } @@ -743,7 +743,7 @@ func TestStreamResume_ConcurrentRepeatedStartsShareOneSession(t *testing.T) { unblock() ack := readUntil(t, second, isAckFor(streaming.TypeStartMission)) - assert.Equal(t, uint8(streaming.ProtocolVersionResume), ack.ProtocolVersion) + assert.Equal(t, uint8(streaming.ProtocolVersionDiscard), ack.ProtocolVersion) assert.Len(t, rh.ops(), 1, "one recording for one mission") assert.Equal(t, 1, rh.h.streamRegistry().activeCount(), "one admission for one mission") From d75b9fcf3bdb432cf6051d256a5b61ce7ac184d4 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:46:37 +0200 Subject: [PATCH 02/14] fix(stream): acknowledge discards only once removed, finish them after the session ended Review fixes: - discard_mission is acknowledged only after the recording is removed: progress acks stop short of its number, as for end_mission. A failed removal closes the connection unacknowledged and marks the row failed, so the producer repeats the discard. - discard_mission without a session, naming a mission id, removes what the server kept of that mission: a partial saved after the resume grace or a supersede, or a row failed by a restart. The extension sends it when it finishes an unconfirmed discard and resume answers resume_unknown. - Discards remove only recordings not saved through end_mission. - Resume replies carry protocolVersion 3, so a producer knows the server takes a discard without a session. - Bump github.com/OCAP2/extension/v5 to the review-fix commit of OCAP2/extension#217; docs updated. --- docs/streaming-architecture.md | 33 ++-- go.mod | 2 +- go.sum | 4 +- internal/server/handler_stream.go | 121 +++++++++++--- .../server/handler_stream_discard_test.go | 152 ++++++++++++++++++ internal/server/handler_stream_resume_test.go | 4 + internal/server/operation.go | 13 +- internal/server/stream_registry.go | 3 + 8 files changed, 293 insertions(+), 39 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index 1c413a89..6a6606f5 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -87,8 +87,10 @@ the next section. a web restart), the extension starts a new segment: it sends `start_mission` again followed by every envelope the server never acknowledged. The skipped numbers mark the new recording `partial`. -- If the mission was already saved but the `end_mission` acknowledgement was - lost, the server answers `resume_completed` instead. +- If the mission was already saved, or discarded, but the `end_mission` or + `discard_mission` acknowledgement was lost, the server answers + `resume_completed` instead. +- Servers at protocol version 3 put `protocolVersion: 3` on every resume reply. - A connection that resumes a mission closes any older connection still bound to it, such as a half-open socket. The extension also abandons a socket whose written envelopes stay unacknowledged for 20 seconds. @@ -153,16 +155,29 @@ whose save was never confirmed are recovered the same way, with their own The addon enforces its save limits, such as `OCAP_settings_minMissionTime`, by not saving a recording. A streamed recording already lives on the server, which would save it as `partial` once its producer is gone. So instead of -`end_mission` the producer sends a numbered `discard_mission` (no payload): +`end_mission` the producer sends a numbered `discard_mission`, whose payload +names the mission (`{"missionId":"…","producerId":"…"}`): - The server removes the recording's `streaming` row and files, along with the - partial segments earlier attempts of the same mission id left behind, and - acknowledges it like `end_mission` (`{"type":"ack","for":"discard_mission"}`). -- The mission id is remembered as finished, so a producer whose acknowledgement - was lost gets `resume_completed`. + partial and failed segments earlier attempts of the same mission id left + behind, and acknowledges it like `end_mission` + (`{"type":"ack","for":"discard_mission","seq":N}`). Recordings saved through + `end_mission` are never removed. +- Like `end_mission`, it is acknowledged only once the recording is gone: until + then progress acknowledgements stop short of its number. If the removal + fails, the server closes the connection without acknowledging it, marks the + row `failed`, and the producer repeats the discard. +- The mission id is remembered as finished for an hour, in memory only, so a + producer whose acknowledgement was lost gets `resume_completed`. After a web + restart it gets `resume_unknown` and repeats the discard, which finds nothing + left to remove. - An unconfirmed discard stays in the extension's spool. Recovery sends only the - `discard_mission` again: a server that no longer holds the mission - (`resume_unknown`) has nothing left to drop, and the mission is not resent. + `discard_mission` again, never the mission data. When the server no longer + holds the session (`resume_unknown`), for example because the resume grace + expired and the mission was saved as `partial`, or the web server restarted + and marked it `failed`, the discard is sent without a session. The server + then removes every recording of that mission id not saved through + `end_mission`. The extension sends `discard_mission` only to servers that acknowledged `start_mission` with `protocolVersion: 3` or later. With an older server it diff --git a/go.mod b/go.mod index 654f8493..6b15217c 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e + github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624 github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index d6d25466..9e09bb8c 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e h1:jvLRAZDjLYx0LoOayo0b2z7/m+lbn5Jjl+B3nVeuyt8= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026193718-a7d2febff16e/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624 h1:5H94K85I2/uZCsLnIhiJtakF0J75nBdJlqgPLAnGLCA= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 2998da35..71e8f501 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -214,12 +214,20 @@ func (h *Handler) streamLoop(ws *websocket.Conn) { case streaming.TypeDiscardMission: if ss == nil { - slog.Info("stream: mission discarded without an active session") + if !h.discardStreamed(envelope.Payload) { + fail("mission could not be discarded") + return + } writeJSON(streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission}) return } - if !h.discardStream(ss, conn, envelope) { - return // another connection took the mission over + switch h.discardStream(ss, conn, envelope) { + case endTakenOver: + return + case endFailed: + current.Store(nil) + fail("mission could not be discarded") + return } current.Store(nil) writeJSON(streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission, Seq: ss.lastSeq.Load()}) @@ -365,22 +373,27 @@ func (h *Handler) takeOver(ss *streamSession, conn *streamConn) bool { } // resumeStream answers resume and returns the session conn now feeds, if any. +// Every reply carries the protocol version: a producer finishing a discard +// learns whether the server takes discard_mission without a session. func (h *Handler) resumeStream(raw json.RawMessage, conn *streamConn) (streaming.AckMessage, *streamSession) { + reply := func(msgType string, seq uint64) streaming.AckMessage { + return streaming.AckMessage{Type: msgType, Seq: seq, ProtocolVersion: streaming.ProtocolVersionDiscard} + } var p streaming.ResumePayload if err := json.Unmarshal(raw, &p); err != nil || p.MissionID == "" { - return streaming.AckMessage{Type: streaming.ServerResumeUnknown}, nil + return reply(streaming.ServerResumeUnknown, 0), nil } reg := h.streamRegistry() if ss := reg.get(p.MissionID); ss != nil && ss.producer == p.ProducerID && h.takeOver(ss, conn) { seq := ss.lastSeq.Load() slog.Info("stream: mission resumed", "missionId", p.MissionID, "seq", seq) - return streaming.AckMessage{Type: streaming.ServerResumeOK, Seq: seq}, ss + return reply(streaming.ServerResumeOK, seq), ss } if reg.isCompleted(p.MissionID) || h.streamEnded(p.MissionID) { - return streaming.AckMessage{Type: streaming.ServerResumeCompleted}, nil + return reply(streaming.ServerResumeCompleted, 0), nil } slog.Warn("stream: resume of unknown mission", "missionId", p.MissionID) - return streaming.AckMessage{Type: streaming.ServerResumeUnknown}, nil + return reply(streaming.ServerResumeUnknown, 0), nil } // acceptSeq applies envelope numbering. It reports false for a duplicate the @@ -535,42 +548,98 @@ func (h *Handler) replaceStreamSegments(ctx context.Context, ss *streamSession) // discardStream drops the session for discard_mission: the producer decided // the recording does not meet its save limits, for example the minimum // mission duration. The streaming row and its files go, along with any -// partial segments an earlier attempt of the mission left behind. It reports -// false when conn no longer feeds the session. -func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope streaming.Envelope) bool { +// partial segments an earlier attempt of the mission left behind. A failure +// leaves the discard unacknowledged, so the producer repeats it. +func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope streaming.Envelope) endResult { ss.mu.Lock() defer ss.mu.Unlock() if ss.owner != conn || ss.done { - return false + return endTakenOver } + // Like end_mission, discard_mission is acknowledged only once the + // recording is gone. + checkSeq(ss, envelope.Seq) ss.done = true - if envelope.Seq > ss.lastSeq.Load() { - ss.lastSeq.Store(envelope.Seq) - } slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) ctx := context.Background() - if err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)); err != nil { - slog.Warn("stream: cannot remove discarded mission", "id", ss.opID, "error", err) + var err error + if hook := streamTestHooks.beforeDiscard; hook != nil { + err = hook() + } + if err == nil { + err = h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) + } + if err == nil && ss.resumable() { + err = h.removeUnendedSegments(ctx, ss.missionID, ss.opID) } if ss.resumable() { - segments, err := h.repoOperation.StreamSegments(ctx, ss.missionID, ss.opID) - if err != nil { - slog.Warn("stream: cannot list earlier segments", "missionId", ss.missionID, "error", err) - } - for _, seg := range segments { - if err := h.removeStreamOperation(ctx, seg.ID, seg.Filename); err != nil { - slog.Warn("stream: cannot remove earlier segment", "id", seg.ID, "error", err) - } - } // Remembered as finished, so a producer whose acknowledgement was // lost learns from resume that the discard went through. - h.streamRegistry().remove(ss, true) + h.streamRegistry().remove(ss, err == nil) } h.streamRegistry().release() + if err != nil { + slog.Error("stream: cannot remove discarded mission", "missionId", ss.missionID, "id", ss.opID, "error", err) + // Not live any more. The producer's repeated discard, without a + // session, removes it. + if ss.opID > 0 { + h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) + } + return endFailed + } + if envelope.Seq > 0 { + ss.lastSeq.Store(envelope.Seq) + } + return endSaved +} + +// discardStreamed handles discard_mission without a session: the producer +// finishes a discard the server did not confirm after the session ended, for +// example when its producer stayed away past the resume grace and the +// mission was saved as partial. What the server kept of the mission goes. +// Recordings saved through end_mission are never touched. It reports false +// when they could not be removed. +func (h *Handler) discardStreamed(raw json.RawMessage) bool { + var p streaming.DiscardMissionPayload + if len(raw) == 0 || json.Unmarshal(raw, &p) != nil || p.MissionID == "" { + slog.Info("stream: mission discarded without an active session") + return true + } + var live int64 + if ss := h.streamRegistry().get(p.MissionID); ss != nil { + live = ss.opID // its owner ends it + } + if err := h.removeUnendedSegments(context.Background(), p.MissionID, live); err != nil { + slog.Error("stream: cannot remove discarded mission", "missionId", p.MissionID, "error", err) + return false + } + slog.Info("stream: mission discarded after its session ended", "missionId", p.MissionID) return true } +// removeUnendedSegments removes the recordings of the producer mission not +// saved through end_mission, except the one with id exceptID. It returns the +// first error and removes what it can. +func (h *Handler) removeUnendedSegments(ctx context.Context, missionID string, exceptID int64) error { + segments, err := h.repoOperation.UnendedStreamSegments(ctx, missionID, exceptID) + if err != nil { + return fmt.Errorf("list segments: %w", err) + } + var first error + for _, seg := range segments { + if err := h.removeStreamOperation(ctx, seg.ID, seg.Filename); err != nil { + slog.Warn("stream: cannot remove segment", "id", seg.ID, "error", err) + if first == nil { + first = err + } + continue + } + slog.Info("stream: removed segment of discarded mission", "missionId", missionID, "removed", seg.Filename) + } + return first +} + // removeStreamOperation deletes a streamed operation row and its files. func (h *Handler) removeStreamOperation(ctx context.Context, id int64, filename string) error { if id > 0 { diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index f0f0d39c..34775a16 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -1,8 +1,11 @@ package server import ( + "context" + "errors" "os" "strings" + "sync" "testing" "time" @@ -88,6 +91,9 @@ func TestStreamDiscard_ResumedSession(t *testing.T) { assert.Empty(t, rh.ops()) assert.Empty(t, rh.recordingFiles()) + reg := rh.h.streamRegistry() + assert.True(t, reg.isCompleted("m-res")) + assert.Zero(t, reg.activeCount()) } func TestStreamDiscard_RemovesEarlierSegments(t *testing.T) { @@ -150,3 +156,149 @@ func TestStreamDiscard_LegacySession(t *testing.T) { assert.Empty(t, rh.recordingFiles()) assert.Zero(t, rh.h.streamRegistry().activeCount()) } + +// discardPayload names a mission in discard_mission. +func discardPayload(missionID, producerID string) streaming.DiscardMissionPayload { + return streaming.DiscardMissionPayload{MissionID: missionID, ProducerID: producerID} +} + +func TestStreamDiscard_AfterSessionEndedRemovesPartial(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Expired", "m-exp", "p-1") + sendStates(t, c, 2, 1, 5) + rh.waitSeq("m-exp", 6) + c.Close() + // The discard never arrived and the grace expired: saved as partial. + require.Eventually(t, func() bool { return rh.h.streamRegistry().get("m-exp") == nil }, 5*time.Second, 10*time.Millisecond) + require.Len(t, rh.opsByMission("m-exp"), 1) + + // The producer's recovery finishes the discard without a session. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-exp", ProducerID: "p-1"}) + reply := readUntil(t, c2, isType(streaming.ServerResumeUnknown)) + assert.Equal(t, uint8(streaming.ProtocolVersionDiscard), reply.ProtocolVersion, "tells the producer it may discard without a session") + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-exp", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + requireClosed(t, c2) + + assert.Empty(t, rh.ops()) + assert.Empty(t, rh.recordingFiles()) +} + +func TestStreamDiscard_AfterRestartRemovesFailedRecording(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Restarted", "m-rst", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-rst", last) + c.Close() + rh.restart() + _, err := FailInterruptedStreams(context.Background(), rh.h.repoOperation) + require.NoError(t, err) + require.Len(t, rh.ops(), 1, "the interrupted recording is kept as failed") + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-rst", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + assert.Empty(t, rh.ops()) +} + +func TestStreamDiscard_AfterSessionEndedKeepsSavedRecording(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + replayMission(t, c, "Saved", "m-saved", "p-1", 3) + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-saved", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + require.Len(t, rh.ops(), 1, "a recording saved through end_mission is never discarded") +} + +func TestStreamDiscard_NotAcknowledgedBeforeRemoval(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Slow", "m-slow-d", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-slow-d", last) + + release := make(chan struct{}) + var releaseOnce sync.Once + unblock := func() { releaseOnce.Do(func() { close(release) }) } + streamTestHooks.beforeDiscard = func() error { <-release; return nil } + t.Cleanup(func() { unblock(); streamTestHooks.beforeDiscard = nil }) + + msgs := make(chan streaming.AckMessage, 64) + go func() { + for { + var m streaming.AckMessage + if c.ReadJSON(&m) != nil { + close(msgs) + return + } + msgs <- m + } + }() + + discardSeq := last + 1 + sendEnv(t, c, streaming.TypeDiscardMission, discardSeq, discardPayload("m-slow-d", "p-1")) + // Several acknowledgement ticks pass while the removal is held. + hold := time.After(300 * time.Millisecond) +holding: + for { + select { + case m := <-msgs: + require.Less(t, m.Seq, discardSeq, "discard_mission acknowledged before the recording was removed: %+v", m) + case <-hold: + break holding + } + } + unblock() + for m := range msgs { + if m.For == streaming.TypeDiscardMission { + assert.Equal(t, discardSeq, m.Seq) + assert.Empty(t, rh.ops()) + return + } + } + t.Fatal("discard_mission was not acknowledged") +} + +func TestStreamDiscard_FailedRemovalIsRepeated(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Locked", "m-fail", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-fail", last) + + streamTestHooks.beforeDiscard = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeDiscard = nil }) + sendEnv(t, c, streaming.TypeDiscardMission, last+1, discardPayload("m-fail", "p-1")) + c.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + var m streaming.AckMessage + err := c.ReadJSON(&m) + if err != nil { + require.True(t, websocket.IsCloseError(err, websocket.CloseInternalServerErr), "%v", err) + break + } + require.NotEqual(t, streaming.TypeDiscardMission, m.For, "a failed discard is not acknowledged") + require.Less(t, m.Seq, last+1) + } + ops := rh.ops() + require.Len(t, ops, 1) + assert.Equal(t, ConversionStatusFailed, ops[0].ConversionStatus, "no longer shown as live") + reg := rh.h.streamRegistry() + assert.False(t, reg.isCompleted("m-fail")) + assert.Zero(t, reg.activeCount()) + streamTestHooks.beforeDiscard = nil + + // The producer repeats the discard: resume_unknown, then without a session. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-fail", ProducerID: "p-1"}) + readUntil(t, c2, isType(streaming.ServerResumeUnknown)) + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-fail", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + assert.Empty(t, rh.ops()) + assert.Empty(t, rh.recordingFiles()) +} diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index aeecfa90..a89949fb 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -611,6 +611,10 @@ func TestStreamResume_SlotsReleasedOnEveryEnding(t *testing.T) { old.Close() superseding := rh.dial() startResumable(t, superseding, "New", "m-new", "p-3") // supersedes m-old + discarded := rh.dial() + startResumable(t, discarded, "Discarded", "m-d", "p-4") + sendEnv(t, discarded, streaming.TypeDiscardMission, 2, nil) + readUntil(t, discarded, isAckFor(streaming.TypeDiscardMission)) require.Eventually(t, func() bool { return reg.activeCount() == 1 }, 5*time.Second, 10*time.Millisecond, "only the newest mission is still active") } diff --git a/internal/server/operation.go b/internal/server/operation.go index dac2899b..7d63eec1 100644 --- a/internal/server/operation.go +++ b/internal/server/operation.go @@ -358,11 +358,22 @@ type StreamSegment struct { // StreamSegments returns the operations recorded for the producer mission, // except the one with id exceptID. func (r *RepoOperation) StreamSegments(ctx context.Context, missionID string, exceptID int64) ([]StreamSegment, error) { + return r.streamSegments(ctx, `stream_mission_id = ? AND id != ?`, missionID, exceptID) +} + +// UnendedStreamSegments returns the operations recorded for the producer +// mission that were not saved through end_mission, except the one with id +// exceptID: partial and failed recordings, and live ones. +func (r *RepoOperation) UnendedStreamSegments(ctx context.Context, missionID string, exceptID int64) ([]StreamSegment, error) { + return r.streamSegments(ctx, `stream_mission_id = ? AND id != ? AND stream_ended = 0`, missionID, exceptID) +} + +func (r *RepoOperation) streamSegments(ctx context.Context, where string, missionID string, exceptID int64) ([]StreamSegment, error) { if missionID == "" { return nil, nil } rows, err := r.db.QueryContext(ctx, - `SELECT id, filename FROM operations WHERE stream_mission_id = ? AND id != ? ORDER BY id`, missionID, exceptID) + `SELECT id, filename FROM operations WHERE `+where+` ORDER BY id`, missionID, exceptID) if err != nil { return nil, err } diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index d53ea739..03531ae3 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -27,6 +27,9 @@ const streamReserveWait = 30 * time.Second var streamTestHooks struct { beforeCreate func() // a start reserved its mission and is about to set it up beforeSave func() // a session is about to be finalized + // beforeDiscard runs before a discarded session is removed; an error + // stands in for a failed removal. + beforeDiscard func() error } // streamConn is one producer WebSocket connection. From 4b9c68553d4b1866efb5ede1e17fb60c2dd051fc Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 00:10:36 +0200 Subject: [PATCH 03/14] fix(stream): lock out starts during a session-less discard, leave live missions to recovered starts Second review round: - A session-less discard reserves the mission id while it removes the mission's recordings, so a start of it cannot slip in. A session of the mission waiting for its producer (same producerId) is dropped in its place; one a connection feeds is left alone and the discard is not acknowledged. A session being finalized is waited for, not exempted. - endStream marks stream_ended before the session leaves the registry, so a discard never takes a just-saved recording for an unended one. - A start marked "recovered" (resent from the extension's spool) no longer supersedes the producer's live mission. - Nil repository guard; tests for the waiting and fed sessions, a discard losing to a supersede, a failed session-less removal, and recovered starts; docs updated. Bump the extension to OCAP2/extension#217's latest commit. --- docs/streaming-architecture.md | 13 +- go.mod | 2 +- go.sum | 4 +- internal/server/handler_stream.go | 128 ++++++++++++------ .../server/handler_stream_discard_test.go | 115 +++++++++++++++- internal/server/handler_stream_resume_test.go | 22 +++ internal/server/stream_registry.go | 4 +- 7 files changed, 238 insertions(+), 50 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index 6a6606f5..b0423f2c 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -97,7 +97,9 @@ the next section. - Sessions are keyed only by mission id, so any number of game servers can stream to one web server. Only a session of the same producer is affected by a new `start_mission`: its earlier mission is saved as `partial` at once instead of - waiting for the grace period. + waiting for the grace period. A start marked `"recovered": true` resends an + older mission from the extension's spool and supersedes nothing: the + producer may be recording its next mission meanwhile. - A session not resumed within the grace period is saved with `partial` appended to its tags. @@ -163,6 +165,8 @@ names the mission (`{"missionId":"…","producerId":"…"}`): behind, and acknowledges it like `end_mission` (`{"type":"ack","for":"discard_mission","seq":N}`). Recordings saved through `end_mission` are never removed. +- It also ends a legacy session (no numbering) the same way; the disconnect that + follows no longer saves it as `partial`. - Like `end_mission`, it is acknowledged only once the recording is gone: until then progress acknowledgements stop short of its number. If the removal fails, the server closes the connection without acknowledging it, marks the @@ -177,7 +181,12 @@ names the mission (`{"missionId":"…","producerId":"…"}`): expired and the mission was saved as `partial`, or the web server restarted and marked it `failed`, the discard is sent without a session. The server then removes every recording of that mission id not saved through - `end_mission`. + `end_mission`, and acknowledges it without `seq`. It holds off a start of the + mission id meanwhile. A session of the mission still waiting for its producer + (`producerId` must match) is dropped in its place. One a connection still + feeds is left alone, and the discard is not acknowledged. +- Files are removed on a best-effort basis, as for a deleted operation: once the + row is gone, the discard counts as done. The extension sends `discard_mission` only to servers that acknowledged `start_mission` with `protocolVersion: 3` or later. With an older server it diff --git a/go.mod b/go.mod index 6b15217c..f4680cad 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624 + github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 9e09bb8c..6221b031 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624 h1:5H94K85I2/uZCsLnIhiJtakF0J75nBdJlqgPLAnGLCA= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026214041-8cb43bf1c624/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd h1:pqJi/A6UrpJZIbYJNwyxAIOj5aY0YMOp7jRnhz+XjGQ= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 71e8f501..5668a665 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -287,9 +287,13 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s return existing, "" } // A producer records one mission at a time: a new start supersedes - // whatever it left behind. Other producers are unaffected. - for _, old := range reg.byProducer(payload.ProducerID, payload.MissionID) { - go h.expireStream(old, 0) + // whatever it left behind. Other producers are unaffected, and so is + // the producer's live mission when the start resends an older one + // from the spool. + if !payload.Recovered { + for _, old := range reg.byProducer(payload.ProducerID, payload.MissionID) { + go h.expireStream(old, 0) + } } } // settle ends the reservation: ss nil when the mission was refused. @@ -505,6 +509,14 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream ss.done = true err := h.saveStream(ss, tag) + ctx := context.Background() + if err == nil && ss.resumable() { + // Marked before the session leaves the registry, so a discard of the + // mission id never takes the saved recording for an unended one. + if err := h.repoOperation.MarkStreamEnded(ctx, ss.opID); err != nil { + slog.Warn("stream: cannot mark mission ended", "error", err) + } + } if ss.resumable() { h.streamRegistry().remove(ss, err == nil) } @@ -514,14 +526,8 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream if envelope.Seq > 0 { ss.lastSeq.Store(envelope.Seq) } - if ss.resumable() { - ctx := context.Background() - if err := h.repoOperation.MarkStreamEnded(ctx, ss.opID); err != nil { - slog.Warn("stream: cannot mark mission ended", "error", err) - } - if !ss.gap { - h.replaceStreamSegments(ctx, ss) - } + if ss.resumable() && !ss.gap { + h.replaceStreamSegments(ctx, ss) } return endSaved } @@ -560,16 +566,23 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st // recording is gone. checkSeq(ss, envelope.Seq) ss.done = true - slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) - - ctx := context.Background() - var err error - if hook := streamTestHooks.beforeDiscard; hook != nil { - err = hook() + if h.dropSession(ss) != nil { + return endFailed } - if err == nil { - err = h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) + if envelope.Seq > 0 { + ss.lastSeq.Store(envelope.Seq) } + return endSaved +} + +// dropSession removes a discarded session's recording and earlier unended +// segments of its mission, and releases the session. On failure the row is +// marked failed, no longer live; the producer's repeated discard, without a +// session, removes it. Caller holds ss.mu and has set ss.done. +func (h *Handler) dropSession(ss *streamSession) error { + slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) + ctx := context.Background() + err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) if err == nil && ss.resumable() { err = h.removeUnendedSegments(ctx, ss.missionID, ss.opID) } @@ -581,47 +594,80 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st h.streamRegistry().release() if err != nil { slog.Error("stream: cannot remove discarded mission", "missionId", ss.missionID, "id", ss.opID, "error", err) - // Not live any more. The producer's repeated discard, without a - // session, removes it. if ss.opID > 0 { h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) } - return endFailed - } - if envelope.Seq > 0 { - ss.lastSeq.Store(envelope.Seq) } - return endSaved + return err } // discardStreamed handles discard_mission without a session: the producer // finishes a discard the server did not confirm after the session ended, for // example when its producer stayed away past the resume grace and the -// mission was saved as partial. What the server kept of the mission goes. -// Recordings saved through end_mission are never touched. It reports false -// when they could not be removed. +// mission was saved as partial. What the server kept of the mission goes; +// recordings saved through end_mission stay. It reports false when they +// could not be removed, or a connection still feeds the mission. func (h *Handler) discardStreamed(raw json.RawMessage) bool { var p streaming.DiscardMissionPayload if len(raw) == 0 || json.Unmarshal(raw, &p) != nil || p.MissionID == "" { slog.Info("stream: mission discarded without an active session") return true } - var live int64 - if ss := h.streamRegistry().get(p.MissionID); ss != nil { - live = ss.opID // its owner ends it + reg := h.streamRegistry() + // Reserving the mission id holds off a start of it while its recordings + // go. A session of the mission is dropped in its place, unless a + // connection still feeds it. + for range 3 { + existing, err := reg.reserve(p.MissionID, streamReserveWait) + if err != nil { + slog.Warn("stream: cannot discard mission, start in progress", "missionId", p.MissionID) + return false + } + if existing == nil { + defer reg.settle(p.MissionID, nil) + if err := h.removeUnendedSegments(context.Background(), p.MissionID, 0); err != nil { + slog.Error("stream: cannot remove discarded mission", "missionId", p.MissionID, "error", err) + return false + } + slog.Info("stream: mission discarded after its session ended", "missionId", p.MissionID) + return true + } + if dropped, retry := h.discardDetached(existing, p.ProducerID); !retry { + return dropped + } } - if err := h.removeUnendedSegments(context.Background(), p.MissionID, live); err != nil { - slog.Error("stream: cannot remove discarded mission", "missionId", p.MissionID, "error", err) - return false + return false +} + +// discardDetached drops a session waiting for its producer to resume, for a +// session-less discard_mission. It refuses one a connection feeds or another +// producer recorded, and asks for a retry when the session was finalized +// meanwhile: it then leaves the registry. +func (h *Handler) discardDetached(ss *streamSession, producer string) (dropped, retry bool) { + ss.mu.Lock() + defer ss.mu.Unlock() + if ss.done { + return false, true } - slog.Info("stream: mission discarded after its session ended", "missionId", p.MissionID) - return true + if ss.owner != nil || ss.producer != producer { + slog.Warn("stream: cannot discard mission, another connection feeds it", "missionId", ss.missionID) + return false, false + } + if ss.grace != nil { + ss.grace.Stop() + ss.grace = nil + } + ss.done = true + return h.dropSession(ss) == nil, false } // removeUnendedSegments removes the recordings of the producer mission not // saved through end_mission, except the one with id exceptID. It returns the // first error and removes what it can. func (h *Handler) removeUnendedSegments(ctx context.Context, missionID string, exceptID int64) error { + if h.repoOperation == nil { + return nil + } segments, err := h.repoOperation.UnendedStreamSegments(ctx, missionID, exceptID) if err != nil { return fmt.Errorf("list segments: %w", err) @@ -640,8 +686,14 @@ func (h *Handler) removeUnendedSegments(ctx context.Context, missionID string, e return first } -// removeStreamOperation deletes a streamed operation row and its files. +// removeStreamOperation deletes a streamed operation row and its files. The +// files go on a best-effort basis, as for a deleted operation. func (h *Handler) removeStreamOperation(ctx context.Context, id int64, filename string) error { + if hook := streamTestHooks.beforeRemove; hook != nil { + if err := hook(); err != nil { + return err + } + } if id > 0 { if err := h.repoOperation.Delete(ctx, id); err != nil && !errors.Is(err, ErrNotFound) { return err diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index 34775a16..70b8d819 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -225,8 +225,8 @@ func TestStreamDiscard_NotAcknowledgedBeforeRemoval(t *testing.T) { release := make(chan struct{}) var releaseOnce sync.Once unblock := func() { releaseOnce.Do(func() { close(release) }) } - streamTestHooks.beforeDiscard = func() error { <-release; return nil } - t.Cleanup(func() { unblock(); streamTestHooks.beforeDiscard = nil }) + streamTestHooks.beforeRemove = func() error { <-release; return nil } + t.Cleanup(func() { unblock(); streamTestHooks.beforeRemove = nil }) msgs := make(chan streaming.AckMessage, 64) go func() { @@ -271,8 +271,8 @@ func TestStreamDiscard_FailedRemovalIsRepeated(t *testing.T) { last := sendStates(t, c, 2, 1, 3) rh.waitSeq("m-fail", last) - streamTestHooks.beforeDiscard = func() error { return errors.New("database is locked") } - t.Cleanup(func() { streamTestHooks.beforeDiscard = nil }) + streamTestHooks.beforeRemove = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeRemove = nil }) sendEnv(t, c, streaming.TypeDiscardMission, last+1, discardPayload("m-fail", "p-1")) c.SetReadDeadline(time.Now().Add(5 * time.Second)) for { @@ -291,7 +291,7 @@ func TestStreamDiscard_FailedRemovalIsRepeated(t *testing.T) { reg := rh.h.streamRegistry() assert.False(t, reg.isCompleted("m-fail")) assert.Zero(t, reg.activeCount()) - streamTestHooks.beforeDiscard = nil + streamTestHooks.beforeRemove = nil // The producer repeats the discard: resume_unknown, then without a session. c2 := rh.dial() @@ -302,3 +302,108 @@ func TestStreamDiscard_FailedRemovalIsRepeated(t *testing.T) { assert.Empty(t, rh.ops()) assert.Empty(t, rh.recordingFiles()) } + +// requireClosedUnacknowledged checks that the server closes the connection +// with an error and never acknowledges discard_mission. +func requireClosedUnacknowledged(t *testing.T, c *websocket.Conn) { + t.Helper() + c.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + var m streaming.AckMessage + if err := c.ReadJSON(&m); err != nil { + return + } + require.NotEqual(t, streaming.TypeDiscardMission, m.For, "discard_mission must not be acknowledged") + } +} + +func TestStreamDiscard_WithoutSessionDropsWaitingSession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Waiting", "m-wait", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-wait", last) + c.Close() + reg := rh.h.streamRegistry() + require.Eventually(t, func() bool { + ss := reg.get("m-wait") + ss.mu.Lock() + defer ss.mu.Unlock() + return ss.owner == nil + }, 5*time.Second, 10*time.Millisecond, "the session waits for its producer") + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-wait", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + + assert.Empty(t, rh.ops()) + assert.Empty(t, rh.recordingFiles()) + assert.Nil(t, reg.get("m-wait")) + assert.True(t, reg.isCompleted("m-wait")) + assert.Zero(t, reg.activeCount()) +} + +func TestStreamDiscard_WithoutSessionLeavesFedSession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Fed", "m-fed", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-fed", last) + + other := rh.dial() + sendEnv(t, other, streaming.TypeDiscardMission, 0, discardPayload("m-fed", "p-1")) + requireClosedUnacknowledged(t, other) + + require.Len(t, rh.ops(), 1, "the live recording stays") + sendEnv(t, c, streaming.TypeEndMission, last+1, nil) + readUntil(t, c, isAckFor(streaming.TypeEndMission)) +} + +func TestStreamDiscard_LosesToSupersede(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Superseded", "m-old-d", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-old-d", last) + + // The producer's next mission supersedes this one; its save is held. + saving := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + unblock := func() { once.Do(func() { close(release) }) } + streamTestHooks.beforeSave = func() { close(saving); <-release } + t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) + next := rh.dial() + sendEnv(t, next, streaming.TypeStartMission, 1, startPayload("Next", "m-next-d", "p-1")) + <-saving + streamTestHooks.beforeSave = nil + + sendEnv(t, c, streaming.TypeDiscardMission, last+1, discardPayload("m-old-d", "p-1")) + unblock() + requireClosedUnacknowledged(t, c) + require.Len(t, rh.opsByMission("m-old-d"), 1, "saved as partial by the supersede") + + // The producer finishes the discard without a session. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-old-d", ProducerID: "p-1"}) + readUntil(t, c2, isType(streaming.ServerResumeUnknown)) + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-old-d", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + assert.Empty(t, rh.opsByMission("m-old-d")) +} + +func TestStreamDiscard_WithoutSessionFailedRemoval(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Expired", "m-exp-f", "p-1") + rh.waitSeq("m-exp-f", sendStates(t, c, 2, 1, 3)) + c.Close() + require.Eventually(t, func() bool { return rh.h.streamRegistry().get("m-exp-f") == nil }, 5*time.Second, 10*time.Millisecond) + + streamTestHooks.beforeRemove = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeRemove = nil }) + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-exp-f", "p-1")) + requireClosedUnacknowledged(t, c2) + require.Len(t, rh.ops(), 1) +} diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index a89949fb..1fa9a23d 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -349,6 +349,28 @@ func TestStreamResume_NewMissionSupersedesProducersOldOne(t *testing.T) { assert.Equal(t, "TvT,partial", old.Tag) } +func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { + rh := newResumeHarness(t, time.Hour) + live := rh.dial() + startResumable(t, live, "Live", "m-live", "p-1") + + // The producer resends an older mission from its spool. + c := rh.dial() + start := startPayload("Spooled", "m-spooled", "p-1") + start["recovered"] = true + sendEnv(t, c, streaming.TypeStartMission, 1, start) + readUntil(t, c, isAckFor(streaming.TypeStartMission)) + sendEnv(t, c, streaming.TypeEndMission, 2, streaming.EndMissionPayload{Interrupted: true}) + readUntil(t, c, isAckFor(streaming.TypeEndMission)) + + ss := rh.h.streamRegistry().get("m-live") + require.NotNil(t, ss, "the live mission is not superseded") + ss.mu.Lock() + defer ss.mu.Unlock() + assert.False(t, ss.done) + assert.NotNil(t, ss.owner, "its connection still feeds it") +} + func TestStreamResume_RepeatedStartReusesSession(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index 03531ae3..1997b2c4 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -27,9 +27,9 @@ const streamReserveWait = 30 * time.Second var streamTestHooks struct { beforeCreate func() // a start reserved its mission and is about to set it up beforeSave func() // a session is about to be finalized - // beforeDiscard runs before a discarded session is removed; an error + // beforeRemove runs before a streamed recording is removed; an error // stands in for a failed removal. - beforeDiscard func() error + beforeRemove func() error } // streamConn is one producer WebSocket connection. From 5599dccb0ad1dc8b6129296eeb533c658fc4038e Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 00:26:25 +0200 Subject: [PATCH 04/14] fix(stream): keep the discarded row until last, exempt recovered sessions from supersede Third review round: - dropSession removes the earlier segments before the session's own row, so a failure leaves that row to be marked failed. - A recovered session is not superseded by its producer's next start either. - Separate logs for a session another producer recorded, and for a session still finalizing after the retries. - Tests: another producer's waiting session, a session-less discard waiting for an expiry save, a failed segment removal, the recovered flag in both directions; the close helpers fail on a read timeout. Docs updated. --- docs/streaming-architecture.md | 24 +++-- internal/server/handler_stream.go | 25 +++-- .../server/handler_stream_discard_test.go | 95 +++++++++++++++++++ internal/server/handler_stream_resume_test.go | 25 +++-- internal/server/stream_registry.go | 7 +- 5 files changed, 150 insertions(+), 26 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index b0423f2c..85a32e12 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -98,8 +98,9 @@ the next section. to one web server. Only a session of the same producer is affected by a new `start_mission`: its earlier mission is saved as `partial` at once instead of waiting for the grace period. A start marked `"recovered": true` resends an - older mission from the extension's spool and supersedes nothing: the - producer may be recording its next mission meanwhile. + older mission from the extension's spool. It supersedes nothing, and the + producer's next start does not supersede it either: the producer may be + recording its next mission meanwhile. - A session not resumed within the grace period is saved with `partial` appended to its tags. @@ -163,14 +164,16 @@ names the mission (`{"missionId":"…","producerId":"…"}`): - The server removes the recording's `streaming` row and files, along with the partial and failed segments earlier attempts of the same mission id left behind, and acknowledges it like `end_mission` - (`{"type":"ack","for":"discard_mission","seq":N}`). Recordings saved through - `end_mission` are never removed. + (`{"type":"ack","for":"discard_mission","seq":N}`). Recordings marked as + saved through `end_mission` (`stream_ended`, set before the session leaves + the registry) are never removed. - It also ends a legacy session (no numbering) the same way; the disconnect that follows no longer saves it as `partial`. - Like `end_mission`, it is acknowledged only once the recording is gone: until then progress acknowledgements stop short of its number. If the removal fails, the server closes the connection without acknowledging it, marks the - row `failed`, and the producer repeats the discard. + session's row `failed` (it is removed last), and the producer repeats the + discard. - The mission id is remembered as finished for an hour, in memory only, so a producer whose acknowledgement was lost gets `resume_completed`. After a web restart it gets `resume_unknown` and repeats the discard, which finds nothing @@ -183,15 +186,16 @@ names the mission (`{"missionId":"…","producerId":"…"}`): then removes every recording of that mission id not saved through `end_mission`, and acknowledges it without `seq`. It holds off a start of the mission id meanwhile. A session of the mission still waiting for its producer - (`producerId` must match) is dropped in its place. One a connection still - feeds is left alone, and the discard is not acknowledged. + (`producerId` must match) is dropped in its place, and one being saved is + waited for. One a connection still feeds, or another producer's, is left + alone, and the discard is not acknowledged. - Files are removed on a best-effort basis, as for a deleted operation: once the row is gone, the discard counts as done. The extension sends `discard_mission` only to servers that acknowledged -`start_mission` with `protocolVersion: 3` or later. With an older server it -closes the connection instead, and that server saves the recording as -`partial`. +`start_mission`, or answered `resume` for the session-less form, with +`protocolVersion: 3` or later. With an older server it closes the connection +instead, and that server saves the recording as `partial`. ### Legacy producers diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 5668a665..75f234fe 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -287,9 +287,9 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s return existing, "" } // A producer records one mission at a time: a new start supersedes - // whatever it left behind. Other producers are unaffected, and so is - // the producer's live mission when the start resends an older one - // from the spool. + // whatever it left behind. Other producers are unaffected. Resends + // from the spool are not the live mission: a recovered start + // supersedes nothing, and is not superseded (see byProducer). if !payload.Recovered { for _, old := range reg.byProducer(payload.ProducerID, payload.MissionID) { go h.expireStream(old, 0) @@ -348,6 +348,7 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s if resumable { ss.missionID = payload.MissionID ss.producer = payload.ProducerID + ss.recovered = payload.Recovered if err := h.repoOperation.SetStreamMission(context.Background(), op.ID, ss.missionID); err != nil { slog.Warn("stream: cannot record mission id", "error", err) } @@ -582,10 +583,15 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st func (h *Handler) dropSession(ss *streamSession) error { slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) ctx := context.Background() - err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) - if err == nil && ss.resumable() { + // The session's own row goes last: after a failure it is still there to + // be marked failed. + var err error + if ss.resumable() { err = h.removeUnendedSegments(ctx, ss.missionID, ss.opID) } + if err == nil { + err = h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) + } if ss.resumable() { // Remembered as finished, so a producer whose acknowledgement was // lost learns from resume that the discard went through. @@ -636,6 +642,7 @@ func (h *Handler) discardStreamed(raw json.RawMessage) bool { return dropped } } + slog.Warn("stream: cannot discard mission, its session is still being finalized", "missionId", p.MissionID) return false } @@ -649,8 +656,12 @@ func (h *Handler) discardDetached(ss *streamSession, producer string) (dropped, if ss.done { return false, true } - if ss.owner != nil || ss.producer != producer { - slog.Warn("stream: cannot discard mission, another connection feeds it", "missionId", ss.missionID) + if ss.owner != nil { + slog.Warn("stream: cannot discard mission, a connection feeds it", "missionId", ss.missionID) + return false, false + } + if ss.producer != producer { + slog.Warn("stream: cannot discard mission of another producer", "missionId", ss.missionID) return false, false } if ss.grace != nil { diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index 70b8d819..114f9002 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -3,6 +3,7 @@ package server import ( "context" "errors" + "net" "os" "strings" "sync" @@ -35,11 +36,20 @@ func requireClosed(t *testing.T, c *websocket.Conn) { c.SetReadDeadline(time.Now().Add(5 * time.Second)) for { if _, _, err := c.ReadMessage(); err != nil { + requireNotTimeout(t, err) return } } } +// requireNotTimeout fails when a read ended by its deadline: the server was +// expected to close the connection. +func requireNotTimeout(t *testing.T, err error) { + t.Helper() + var netErr net.Error + require.False(t, errors.As(err, &netErr) && netErr.Timeout(), "the server did not close the connection: %v", err) +} + func TestStreamDiscard_RemovesRecording(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() @@ -311,6 +321,7 @@ func requireClosedUnacknowledged(t *testing.T, c *websocket.Conn) { for { var m streaming.AckMessage if err := c.ReadJSON(&m); err != nil { + requireNotTimeout(t, err) return } require.NotEqual(t, streaming.TypeDiscardMission, m.For, "discard_mission must not be acknowledged") @@ -343,6 +354,90 @@ func TestStreamDiscard_WithoutSessionDropsWaitingSession(t *testing.T) { assert.Zero(t, reg.activeCount()) } +// detachedSession waits until the mission's session waits for its producer. +func (rh *resumeHarness) detachedSession(missionID string) { + rh.t.Helper() + require.Eventually(rh.t, func() bool { + ss := rh.h.streamRegistry().get(missionID) + if ss == nil { + return false + } + ss.mu.Lock() + defer ss.mu.Unlock() + return ss.owner == nil + }, 5*time.Second, 10*time.Millisecond) +} + +func TestStreamDiscard_WithoutSessionLeavesOtherProducersSession(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Theirs", "m-theirs", "p-1") + rh.waitSeq("m-theirs", sendStates(t, c, 2, 1, 3)) + c.Close() + rh.detachedSession("m-theirs") + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-theirs", "p-2")) + requireClosedUnacknowledged(t, c2) + require.Len(t, rh.ops(), 1) + assert.NotNil(t, rh.h.streamRegistry().get("m-theirs"), "its producer may still resume it") +} + +func TestStreamDiscard_WithoutSessionWaitsForExpiry(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Expiring", "m-expiring", "p-1") + rh.waitSeq("m-expiring", sendStates(t, c, 2, 1, 3)) + c.Close() + rh.detachedSession("m-expiring") + ss := rh.h.streamRegistry().get("m-expiring") + + // The grace expires and the partial is being saved when the discard + // arrives: it waits for the save, then removes the partial. + saving := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + unblock := func() { once.Do(func() { close(release) }) } + streamTestHooks.beforeSave = func() { close(saving); <-release } + t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) + go rh.h.expireStream(ss, 0) + <-saving + streamTestHooks.beforeSave = nil + + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-expiring", "p-1")) + time.Sleep(100 * time.Millisecond) // the discard blocks on the session + unblock() + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + assert.Empty(t, rh.ops(), "the partial saved meanwhile is removed") +} + +func TestStreamDiscard_FailedSegmentRemovalMarksRowFailed(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Earlier", "m-part", "p-1") + rh.waitSeq("m-part", sendStates(t, c, 2, 1, 3)) + c.Close() + require.Eventually(t, func() bool { return rh.h.streamRegistry().get("m-part") == nil }, 5*time.Second, 10*time.Millisecond) + + // A resend of the mission is discarded, but its earlier segment cannot go. + c2 := rh.dial() + startResumable(t, c2, "Resent", "m-part", "p-1") + last := sendStates(t, c2, 2, 1, 3) + rh.waitSeq("m-part", last) + streamTestHooks.beforeRemove = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeRemove = nil }) + sendEnv(t, c2, streaming.TypeDiscardMission, last+1, discardPayload("m-part", "p-1")) + requireClosedUnacknowledged(t, c2) + + statuses := map[string]string{} + for _, op := range rh.ops() { + statuses[op.MissionName] = op.ConversionStatus + } + assert.Len(t, statuses, 2, "nothing was removed") + assert.Equal(t, ConversionStatusFailed, statuses["Resent"], "the discarded session's row is no longer live") +} + func TestStreamDiscard_WithoutSessionLeavesFedSession(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index 1fa9a23d..3b588bb0 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -354,21 +354,32 @@ func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { live := rh.dial() startResumable(t, live, "Live", "m-live", "p-1") + reg := rh.h.streamRegistry() + running := func(missionID string) bool { + ss := reg.get(missionID) + if ss == nil { + return false + } + ss.mu.Lock() + defer ss.mu.Unlock() + return !ss.done && ss.owner != nil + } + // The producer resends an older mission from its spool. c := rh.dial() start := startPayload("Spooled", "m-spooled", "p-1") start["recovered"] = true sendEnv(t, c, streaming.TypeStartMission, 1, start) readUntil(t, c, isAckFor(streaming.TypeStartMission)) + assert.Never(t, func() bool { return !running("m-live") }, 300*time.Millisecond, 10*time.Millisecond, "the live mission is not superseded") + + // The producer's next live mission supersedes the live one, not the resend. + next := rh.dial() + startResumable(t, next, "Next", "m-next", "p-1") + require.Eventually(t, func() bool { return reg.get("m-live") == nil }, 5*time.Second, 10*time.Millisecond) + assert.Never(t, func() bool { return !running("m-spooled") }, 300*time.Millisecond, 10*time.Millisecond, "the resend is not superseded") sendEnv(t, c, streaming.TypeEndMission, 2, streaming.EndMissionPayload{Interrupted: true}) readUntil(t, c, isAckFor(streaming.TypeEndMission)) - - ss := rh.h.streamRegistry().get("m-live") - require.NotNil(t, ss, "the live mission is not superseded") - ss.mu.Lock() - defer ss.mu.Unlock() - assert.False(t, ss.done) - assert.NotNil(t, ss.owner, "its connection still feeds it") } func TestStreamResume_RepeatedStartReusesSession(t *testing.T) { diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index 1997b2c4..eac919b2 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -55,6 +55,7 @@ type streamSession struct { mu sync.Mutex missionID string // empty for legacy sessions producer string + recovered bool // started by a resend from the producer's spool session *ingestion.Session outputDir string opID int64 @@ -188,13 +189,15 @@ func (r *streamRegistry) isCompleted(missionID string) bool { return ok && time.Since(at) <= completedTTL } -// byProducer returns the producer's active sessions except keep. +// byProducer returns the producer's active sessions except keep, and except +// recovered ones: a resend from the spool is not the mission the producer +// records live, which a new start supersedes. func (r *streamRegistry) byProducer(producer string, keep string) []*streamSession { r.mu.Lock() defer r.mu.Unlock() var out []*streamSession for id, s := range r.sessions { - if id != keep && s.producer == producer { + if id != keep && s.producer == producer && !s.recovered { out = append(out, s) } } From 36b74497d3fd2af09a7521edbcc93e3aa2af307b Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 00:38:34 +0200 Subject: [PATCH 05/14] chore(deps): bump extension to the latest commit of OCAP2/extension#217 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index f4680cad..767e4321 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd + github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 6221b031..a738ed68 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd h1:pqJi/A6UrpJZIbYJNwyxAIOj5aY0YMOp7jRnhz+XjGQ= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026220824-cd71933d63fd/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f h1:fH/jJ2Wzmp4yT6QfqcDv7QZcSzwX8QOo52dBEZ/BOfE= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= From f77249a629bcb248ecd524e9a0f0ba19fde9d9c6 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 01:02:54 +0200 Subject: [PATCH 06/14] fix(stream): drop abandoned missions that recorded nothing, make the discard race tests deterministic Fourth review round: - A session superseded or expired before it received anything after its start is removed instead of saved as an empty partial. - The session discard ack carries seq only for resumable sessions, as for end_mission; a failed stream_ended mark is logged as an error. - Session-less discard settles its reservation explicitly. - Tests: the supersede race calls discardStream after the supersede, the expiry race waits on a hook instead of sleeping, a session-less discard during an end save keeps the recording, a start waits for a session-less discard's reservation, empty missions are dropped; the harness retires leftover sessions so grace timers never fire into later tests. - Docs: protocol 2 "or later"; bump the extension to OCAP2/extension#217's latest commit. --- docs/streaming-architecture.md | 7 +- go.mod | 2 +- go.sum | 4 +- internal/server/handler_stream.go | 34 +++++- .../server/handler_stream_discard_test.go | 103 ++++++++++++++---- internal/server/handler_stream_resume_test.go | 45 +++++++- internal/server/stream_registry.go | 3 + 7 files changed, 163 insertions(+), 35 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index 85a32e12..ea42bea1 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -66,7 +66,7 @@ the connection without a success acknowledgement, and the row is marked failed. ### Resume after a network failure -Producers that declare `protocolVersion: 2` in `start_mission` get lossless +Producers that declare `protocolVersion: 2` or later in `start_mission` get lossless recovery from network failures. Older producers keep the behavior described in the next section. @@ -102,7 +102,8 @@ the next section. producer's next start does not supersede it either: the producer may be recording its next mission meanwhile. - A session not resumed within the grace period is saved with `partial` appended - to its tags. + to its tags. A session superseded or expired before it received anything + after `start_mission` is removed instead: an empty recording is not kept. | Setting | Default | Meaning | |---|---|---| @@ -199,7 +200,7 @@ instead, and that server saves the recording as `partial`. ### Legacy producers -For producers without `protocolVersion: 2`, disconnecting without `end_mission`, +For producers without `protocolVersion: 2` or later, disconnecting without `end_mission`, including an addon/extension crash or a normal WebSocket close, finalizes received data and appends `partial` to the mission's existing tags. Repeated starts cannot silently discard an active mission. Their reconnect starts a new diff --git a/go.mod b/go.mod index 767e4321..74f2b5ff 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f + github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index a738ed68..8b00f13c 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f h1:fH/jJ2Wzmp4yT6QfqcDv7QZcSzwX8QOo52dBEZ/BOfE= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026223734-6cad419f840f/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb h1:cYkeq0D7ZQ6xNgYSRTx9ht6QUwD9CaCe75iaP8NV1Oo= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 75f234fe..b18b6315 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -230,7 +230,11 @@ func (h *Handler) streamLoop(ws *websocket.Conn) { return } current.Store(nil) - writeJSON(streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission, Seq: ss.lastSeq.Load()}) + ack := streaming.AckMessage{Type: streaming.ServerAck, For: streaming.TypeDiscardMission} + if ss.resumable() { + ack.Seq = ss.lastSeq.Load() + } + writeJSON(ack) return default: @@ -515,7 +519,8 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream // Marked before the session leaves the registry, so a discard of the // mission id never takes the saved recording for an unended one. if err := h.repoOperation.MarkStreamEnded(ctx, ss.opID); err != nil { - slog.Warn("stream: cannot mark mission ended", "error", err) + // A discard of the mission id would now take it for unended. + slog.Error("stream: cannot mark mission ended", "missionId", ss.missionID, "id", ss.opID, "error", err) } } if ss.resumable() { @@ -630,14 +635,18 @@ func (h *Handler) discardStreamed(raw json.RawMessage) bool { return false } if existing == nil { - defer reg.settle(p.MissionID, nil) - if err := h.removeUnendedSegments(context.Background(), p.MissionID, 0); err != nil { + err := h.removeUnendedSegments(context.Background(), p.MissionID, 0) + reg.settle(p.MissionID, nil) + if err != nil { slog.Error("stream: cannot remove discarded mission", "missionId", p.MissionID, "error", err) return false } slog.Info("stream: mission discarded after its session ended", "missionId", p.MissionID) return true } + if hook := streamTestHooks.beforeDiscardSession; hook != nil { + hook() + } if dropped, retry := h.discardDetached(existing, p.ProducerID); !retry { return dropped } @@ -777,8 +786,23 @@ func (h *Handler) expireStream(ss *streamSession, gen uint64) { if gen == 0 { reason = "superseded by a new mission" } - slog.Warn("stream: saving abandoned mission as partial", "missionId", ss.missionID, "reason", reason) ss.done = true + if ss.lastSeq.Load() <= 1 { + // Nothing was recorded after the start, for example a start the + // producer gave up waiting for: an empty recording is not kept. + slog.Info("stream: dropping abandoned mission that recorded nothing", "missionId", ss.missionID, "reason", reason) + ctx := context.Background() + if err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)); err != nil { + slog.Error("stream: cannot remove empty mission", "missionId", ss.missionID, "id", ss.opID, "error", err) + if ss.opID > 0 { + h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) + } + } + h.streamRegistry().remove(ss, false) + h.streamRegistry().release() + return + } + slog.Warn("stream: saving abandoned mission as partial", "missionId", ss.missionID, "reason", reason) _ = h.saveStream(ss, "partial") h.streamRegistry().remove(ss, false) } diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index 114f9002..a4366c5e 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -336,12 +336,7 @@ func TestStreamDiscard_WithoutSessionDropsWaitingSession(t *testing.T) { rh.waitSeq("m-wait", last) c.Close() reg := rh.h.streamRegistry() - require.Eventually(t, func() bool { - ss := reg.get("m-wait") - ss.mu.Lock() - defer ss.mu.Unlock() - return ss.owner == nil - }, 5*time.Second, 10*time.Millisecond, "the session waits for its producer") + rh.detachedSession("m-wait") c2 := rh.dial() sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-wait", "p-1")) @@ -404,14 +399,82 @@ func TestStreamDiscard_WithoutSessionWaitsForExpiry(t *testing.T) { <-saving streamTestHooks.beforeSave = nil + reached := make(chan struct{}) + var reachedOnce sync.Once + streamTestHooks.beforeDiscardSession = func() { reachedOnce.Do(func() { close(reached) }) } + t.Cleanup(func() { streamTestHooks.beforeDiscardSession = nil }) c2 := rh.dial() sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-expiring", "p-1")) - time.Sleep(100 * time.Millisecond) // the discard blocks on the session + <-reached // the discard found the session being saved unblock() readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) assert.Empty(t, rh.ops(), "the partial saved meanwhile is removed") } +func TestStreamDiscard_WithoutSessionKeepsRecordingSavedMeanwhile(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Saved", "m-saving", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-saving", last) + + // end_mission is being saved when a session-less discard of the + // mission arrives: the saved recording stays. + saving := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + unblock := func() { once.Do(func() { close(release) }) } + streamTestHooks.beforeSave = func() { close(saving); <-release } + t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) + sendEnv(t, c, streaming.TypeEndMission, last+1, nil) + <-saving + streamTestHooks.beforeSave = nil + + reached := make(chan struct{}) + var reachedOnce sync.Once + streamTestHooks.beforeDiscardSession = func() { reachedOnce.Do(func() { close(reached) }) } + t.Cleanup(func() { streamTestHooks.beforeDiscardSession = nil }) + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-saving", "p-1")) + <-reached + unblock() + readUntil(t, c, isAckFor(streaming.TypeEndMission)) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + require.Len(t, rh.ops(), 1, "a recording saved through end_mission is never discarded") +} + +func TestStreamDiscard_WithoutSessionHoldsOffStart(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Expired", "m-held", "p-1") + rh.waitSeq("m-held", sendStates(t, c, 2, 1, 3)) + c.Close() + require.Eventually(t, func() bool { return rh.h.streamRegistry().get("m-held") == nil }, 5*time.Second, 10*time.Millisecond) + + // The session-less discard is removing the partial when a start of the + // same mission arrives: the start waits for it. + removing := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + unblock := func() { once.Do(func() { close(release) }) } + streamTestHooks.beforeRemove = func() error { close(removing); <-release; return nil } + t.Cleanup(func() { unblock(); streamTestHooks.beforeRemove = nil }) + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-held", "p-1")) + <-removing + streamTestHooks.beforeRemove = nil + + c3 := rh.dial() + sendEnv(t, c3, streaming.TypeStartMission, 1, startPayload("Restarted", "m-held", "p-1")) + assert.Never(t, func() bool { return rh.h.streamRegistry().get("m-held") != nil }, 200*time.Millisecond, 10*time.Millisecond, "the start waits for the discard") + unblock() + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + readUntil(t, c3, isAckFor(streaming.TypeStartMission)) + ops := rh.ops() + require.Len(t, ops, 1, "only the new start's recording is left") + assert.Equal(t, "Restarted", ops[0].MissionName) +} + func TestStreamDiscard_FailedSegmentRemovalMarksRowFailed(t *testing.T) { rh := newResumeHarness(t, 100*time.Millisecond) c := rh.dial() @@ -459,22 +522,16 @@ func TestStreamDiscard_LosesToSupersede(t *testing.T) { c := rh.dial() startResumable(t, c, "Superseded", "m-old-d", "p-1") last := sendStates(t, c, 2, 1, 3) - rh.waitSeq("m-old-d", last) - - // The producer's next mission supersedes this one; its save is held. - saving := make(chan struct{}) - release := make(chan struct{}) - var once sync.Once - unblock := func() { once.Do(func() { close(release) }) } - streamTestHooks.beforeSave = func() { close(saving); <-release } - t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) - next := rh.dial() - sendEnv(t, next, streaming.TypeStartMission, 1, startPayload("Next", "m-next-d", "p-1")) - <-saving - streamTestHooks.beforeSave = nil - - sendEnv(t, c, streaming.TypeDiscardMission, last+1, discardPayload("m-old-d", "p-1")) - unblock() + ss := rh.waitSeq("m-old-d", last) + ss.mu.Lock() + owner := ss.owner + ss.mu.Unlock() + + // The producer's next mission supersedes this one before its discard is + // handled: the discard finds the session taken and is not acknowledged. + rh.h.expireStream(ss, 0) + env := streaming.Envelope{Type: streaming.TypeDiscardMission, Seq: last + 1} + assert.Equal(t, endTakenOver, rh.h.discardStream(ss, owner, env)) requireClosedUnacknowledged(t, c) require.Len(t, rh.opsByMission("m-old-d"), 1, "saved as partial by the supersede") diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index 3b588bb0..009860c6 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -33,7 +33,30 @@ func newResumeHarness(t *testing.T, grace time.Duration) *resumeHarness { h.setting.Streaming.AckInterval = 20 * time.Millisecond srv := httptest.NewServer(mux) t.Cleanup(srv.Close) - return &resumeHarness{t: t, h: h, srv: srv, dir: dir} + rh := &resumeHarness{t: t, h: h, srv: srv, dir: dir} + t.Cleanup(func() { retire(rh.h) }) + return rh +} + +// retire keeps a handler's sessions from ever being finalized, so a grace +// timer a test leaves behind cannot fire into a later test's hooks. +func retire(h *Handler) { + reg := h.streamRegistry() + reg.mu.Lock() + sessions := make([]*streamSession, 0, len(reg.sessions)) + for _, ss := range reg.sessions { + sessions = append(sessions, ss) + } + reg.mu.Unlock() + for _, ss := range sessions { + ss.mu.Lock() + ss.done = true + if ss.grace != nil { + ss.grace.Stop() + ss.grace = nil + } + ss.mu.Unlock() + } } func (rh *resumeHarness) dial() *websocket.Conn { @@ -330,6 +353,7 @@ func TestStreamResume_NewMissionSupersedesProducersOldOne(t *testing.T) { rh := newResumeHarness(t, time.Hour) c := rh.dial() startResumable(t, c, "Old", "m-old", "p-1") + rh.waitSeq("m-old", sendStates(t, c, 2, 1, 3)) other := rh.dial() startResumable(t, other, "Other", "m-other", "p-2") c.Close() // waiting for resume, an hour of grace @@ -349,6 +373,24 @@ func TestStreamResume_NewMissionSupersedesProducersOldOne(t *testing.T) { assert.Equal(t, "TvT,partial", old.Tag) } +func TestStreamResume_AbandonedEmptyMissionIsDropped(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + c := rh.dial() + startResumable(t, c, "Expired empty", "m-empty-x", "p-1") + c.Close() + empty := rh.dial() + startResumable(t, empty, "Superseded empty", "m-empty-s", "p-2") + next := rh.dial() + startResumable(t, next, "Next", "m-next-e", "p-2") + + reg := rh.h.streamRegistry() + require.Eventually(t, func() bool { return reg.get("m-empty-x") == nil && reg.get("m-empty-s") == nil }, 5*time.Second, 10*time.Millisecond) + ops := rh.ops() + require.Len(t, ops, 1, "neither the expired nor the superseded empty mission is kept") + assert.Equal(t, "Next", ops[0].MissionName) + assert.Equal(t, 1, reg.activeCount()) +} + func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { rh := newResumeHarness(t, time.Hour) live := rh.dial() @@ -438,6 +480,7 @@ func TestStreamRegistryCompletedExpires(t *testing.T) { func (rh *resumeHarness) restart() { rh.t.Helper() rh.srv.Close() + retire(rh.h) // the old process is gone h := &Handler{repoOperation: rh.h.repoOperation, setting: rh.h.setting} mux := http.NewServeMux() mux.HandleFunc("GET /api/v1/stream", h.HandleStream) diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index eac919b2..c608c115 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -30,6 +30,9 @@ var streamTestHooks struct { // beforeRemove runs before a streamed recording is removed; an error // stands in for a failed removal. beforeRemove func() error + // beforeDiscardSession runs before a session-less discard_mission + // handles a session of its mission. + beforeDiscardSession func() } // streamConn is one producer WebSocket connection. From 80fa413c640bd43258c035a2478b85a8740ff0e0 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 01:40:03 +0200 Subject: [PATCH 07/14] fix(stream): judge empty sessions by what they received, refuse unreadable session-less discards Fifth review round: - An abandoned session counts as empty when it accepted no envelope after its start, not by its sequence number, which unnumbered envelopes do not advance. - A session-less discard_mission whose payload does not name a mission is not acknowledged, so the producer keeps its spool; an empty payload is still acknowledged, as for end_mission. - dropSession and the empty-session drop release the slot before leaving the registry, as saveStream does. - Tests: an unreadable session-less payload, a session discard keeping the mission's saved recording, a failed removal of an empty session; hooks are once-safe gates. Bump the extension to OCAP2/extension#217's latest commit. --- go.mod | 2 +- go.sum | 4 +- internal/server/handler_stream.go | 18 ++- .../server/handler_stream_discard_test.go | 121 +++++++++++++----- 4 files changed, 101 insertions(+), 44 deletions(-) diff --git a/go.mod b/go.mod index 74f2b5ff..7c7d2048 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb + github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124 github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 8b00f13c..238f079c 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb h1:cYkeq0D7ZQ6xNgYSRTx9ht6QUwD9CaCe75iaP8NV1Oo= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026230020-3d996d0dd0fb/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124 h1:GKfqwuNOsVYX8D8OhjiGtcAzvU1fEgqqjLmxwlHx4Fc= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index b18b6315..cb85edb8 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -597,12 +597,12 @@ func (h *Handler) dropSession(ss *streamSession) error { if err == nil { err = h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)) } + h.streamRegistry().release() // before leaving the registry, as saveStream does if ss.resumable() { // Remembered as finished, so a producer whose acknowledgement was // lost learns from resume that the discard went through. h.streamRegistry().remove(ss, err == nil) } - h.streamRegistry().release() if err != nil { slog.Error("stream: cannot remove discarded mission", "missionId", ss.missionID, "id", ss.opID, "error", err) if ss.opID > 0 { @@ -620,9 +620,15 @@ func (h *Handler) dropSession(ss *streamSession) error { // could not be removed, or a connection still feeds the mission. func (h *Handler) discardStreamed(raw json.RawMessage) bool { var p streaming.DiscardMissionPayload - if len(raw) == 0 || json.Unmarshal(raw, &p) != nil || p.MissionID == "" { + if len(raw) == 0 || string(raw) == "null" { slog.Info("stream: mission discarded without an active session") - return true + return true // like end_mission without a session: nothing named + } + if err := json.Unmarshal(raw, &p); err != nil || p.MissionID == "" { + // Not acknowledged: the producer keeps its spool rather than + // believing the mission dropped. + slog.Warn("stream: invalid discard_mission payload", "error", err) + return false } reg := h.streamRegistry() // Reserving the mission id holds off a start of it while its recordings @@ -787,8 +793,8 @@ func (h *Handler) expireStream(ss *streamSession, gen uint64) { reason = "superseded by a new mission" } ss.done = true - if ss.lastSeq.Load() <= 1 { - // Nothing was recorded after the start, for example a start the + if len(ss.counts) == 0 { + // Nothing was received after the start, for example a start the // producer gave up waiting for: an empty recording is not kept. slog.Info("stream: dropping abandoned mission that recorded nothing", "missionId", ss.missionID, "reason", reason) ctx := context.Background() @@ -798,8 +804,8 @@ func (h *Handler) expireStream(ss *streamSession, gen uint64) { h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) } } - h.streamRegistry().remove(ss, false) h.streamRegistry().release() + h.streamRegistry().remove(ss, false) return } slog.Warn("stream: saving abandoned mission as partial", "missionId", ss.missionID, "reason", reason) diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index a4366c5e..3f6b3247 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -167,6 +167,28 @@ func TestStreamDiscard_LegacySession(t *testing.T) { assert.Zero(t, rh.h.streamRegistry().activeCount()) } +// gate holds the hook calls it is installed in until it opens; reached +// closes on the first call. Any number of calls is safe, including a stray +// one from a session an earlier test left behind. +type gate struct { + reached chan struct{} + release chan struct{} + reachOnce, openOnce sync.Once +} + +func newGate(t *testing.T) *gate { + g := &gate{reached: make(chan struct{}), release: make(chan struct{})} + t.Cleanup(g.open) + return g +} + +func (g *gate) hold() { + g.reachOnce.Do(func() { close(g.reached) }) + <-g.release +} + +func (g *gate) open() { g.openOnce.Do(func() { close(g.release) }) } + // discardPayload names a mission in discard_mission. func discardPayload(missionID, producerID string) streaming.DiscardMissionPayload { return streaming.DiscardMissionPayload{MissionID: missionID, ProducerID: producerID} @@ -389,24 +411,20 @@ func TestStreamDiscard_WithoutSessionWaitsForExpiry(t *testing.T) { // The grace expires and the partial is being saved when the discard // arrives: it waits for the save, then removes the partial. - saving := make(chan struct{}) - release := make(chan struct{}) - var once sync.Once - unblock := func() { once.Do(func() { close(release) }) } - streamTestHooks.beforeSave = func() { close(saving); <-release } - t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) + save := newGate(t) + streamTestHooks.beforeSave = save.hold + t.Cleanup(func() { streamTestHooks.beforeSave = nil }) go rh.h.expireStream(ss, 0) - <-saving - streamTestHooks.beforeSave = nil + <-save.reached - reached := make(chan struct{}) - var reachedOnce sync.Once - streamTestHooks.beforeDiscardSession = func() { reachedOnce.Do(func() { close(reached) }) } + found := newGate(t) + found.open() // only notes that the discard got there + streamTestHooks.beforeDiscardSession = found.hold t.Cleanup(func() { streamTestHooks.beforeDiscardSession = nil }) c2 := rh.dial() sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-expiring", "p-1")) - <-reached // the discard found the session being saved - unblock() + <-found.reached // the discard found the session being saved + save.open() readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) assert.Empty(t, rh.ops(), "the partial saved meanwhile is removed") } @@ -420,24 +438,20 @@ func TestStreamDiscard_WithoutSessionKeepsRecordingSavedMeanwhile(t *testing.T) // end_mission is being saved when a session-less discard of the // mission arrives: the saved recording stays. - saving := make(chan struct{}) - release := make(chan struct{}) - var once sync.Once - unblock := func() { once.Do(func() { close(release) }) } - streamTestHooks.beforeSave = func() { close(saving); <-release } - t.Cleanup(func() { unblock(); streamTestHooks.beforeSave = nil }) + save := newGate(t) + streamTestHooks.beforeSave = save.hold + t.Cleanup(func() { streamTestHooks.beforeSave = nil }) sendEnv(t, c, streaming.TypeEndMission, last+1, nil) - <-saving - streamTestHooks.beforeSave = nil + <-save.reached - reached := make(chan struct{}) - var reachedOnce sync.Once - streamTestHooks.beforeDiscardSession = func() { reachedOnce.Do(func() { close(reached) }) } + found := newGate(t) + found.open() + streamTestHooks.beforeDiscardSession = found.hold t.Cleanup(func() { streamTestHooks.beforeDiscardSession = nil }) c2 := rh.dial() sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-saving", "p-1")) - <-reached - unblock() + <-found.reached + save.open() readUntil(t, c, isAckFor(streaming.TypeEndMission)) readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) require.Len(t, rh.ops(), 1, "a recording saved through end_mission is never discarded") @@ -453,21 +467,17 @@ func TestStreamDiscard_WithoutSessionHoldsOffStart(t *testing.T) { // The session-less discard is removing the partial when a start of the // same mission arrives: the start waits for it. - removing := make(chan struct{}) - release := make(chan struct{}) - var once sync.Once - unblock := func() { once.Do(func() { close(release) }) } - streamTestHooks.beforeRemove = func() error { close(removing); <-release; return nil } - t.Cleanup(func() { unblock(); streamTestHooks.beforeRemove = nil }) + remove := newGate(t) + streamTestHooks.beforeRemove = func() error { remove.hold(); return nil } + t.Cleanup(func() { streamTestHooks.beforeRemove = nil }) c2 := rh.dial() sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-held", "p-1")) - <-removing - streamTestHooks.beforeRemove = nil + <-remove.reached c3 := rh.dial() sendEnv(t, c3, streaming.TypeStartMission, 1, startPayload("Restarted", "m-held", "p-1")) assert.Never(t, func() bool { return rh.h.streamRegistry().get("m-held") != nil }, 200*time.Millisecond, 10*time.Millisecond, "the start waits for the discard") - unblock() + remove.open() readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) readUntil(t, c3, isAckFor(streaming.TypeStartMission)) ops := rh.ops() @@ -559,3 +569,44 @@ func TestStreamDiscard_WithoutSessionFailedRemoval(t *testing.T) { requireClosedUnacknowledged(t, c2) require.Len(t, rh.ops(), 1) } + +func TestStreamDiscard_WithoutSessionInvalidPayload(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + for _, payload := range []any{map[string]any{"missionId": 7}, map[string]any{"producerId": "p-1"}} { + c := rh.dial() + sendEnv(t, c, streaming.TypeDiscardMission, 0, payload) + requireClosedUnacknowledged(t, c) // the producer keeps its spool + } +} + +func TestStreamDiscard_KeepsSavedRecordingOfTheMission(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + replayMission(t, c, "Saved", "m-twice", "p-1", 3) + + // A later attempt of the same mission id is discarded on its session. + c2 := rh.dial() + startResumable(t, c2, "Again", "m-twice", "p-1") + last := sendStates(t, c2, 2, 1, 2) + sendEnv(t, c2, streaming.TypeDiscardMission, last+1, discardPayload("m-twice", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + ops := rh.ops() + require.Len(t, ops, 1, "the recording saved through end_mission stays") + assert.Equal(t, "Saved", ops[0].MissionName) +} + +func TestStreamDiscard_EmptyMissionRemovalFailure(t *testing.T) { + rh := newResumeHarness(t, 100*time.Millisecond) + streamTestHooks.beforeRemove = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeRemove = nil }) + c := rh.dial() + startResumable(t, c, "Empty", "m-empty-f", "p-1") + c.Close() + + reg := rh.h.streamRegistry() + require.Eventually(t, func() bool { return reg.get("m-empty-f") == nil }, 5*time.Second, 10*time.Millisecond) + assert.Zero(t, reg.activeCount(), "the slot is released") + ops := rh.ops() + require.Len(t, ops, 1) + assert.Equal(t, ConversionStatusFailed, ops[0].ConversionStatus, "no longer shown as live") +} From f5fb830a3f41903c4cbe4e7149062d190b540774 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 01:40:15 +0200 Subject: [PATCH 08/14] style: gofmt the test gate --- internal/server/handler_stream_discard_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index 3f6b3247..759aa027 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -171,8 +171,8 @@ func TestStreamDiscard_LegacySession(t *testing.T) { // closes on the first call. Any number of calls is safe, including a stray // one from a session an earlier test left behind. type gate struct { - reached chan struct{} - release chan struct{} + reached chan struct{} + release chan struct{} reachOnce, openOnce sync.Once } From 4bc2ade613c39d2618e1286e8c85add5109cadd1 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:00:09 +0200 Subject: [PATCH 09/14] test(stream): pin the protocol version on resume replies, cover a recovered start next to a partial save Sixth review round (nits only): - Assert protocolVersion 3 on resume_ok and resume_completed, which the extension reads to decide whether a pending discard can still go through. - The recovered-start test records on the live mission, so its supersede saves a partial, and checks both recordings. - "discarding mission" is logged before the removal, so a failure no longer reads as a success. - Bump the extension to OCAP2/extension#217's latest commit. --- go.mod | 2 +- go.sum | 4 ++-- internal/server/handler_stream.go | 2 +- internal/server/handler_stream_discard_test.go | 4 +++- internal/server/handler_stream_resume_test.go | 8 ++++++++ 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 7c7d2048..33321e42 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124 + github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 238f079c..8422dc71 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124 h1:GKfqwuNOsVYX8D8OhjiGtcAzvU1fEgqqjLmxwlHx4Fc= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026232226-bc87c27c4124/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a h1:F8rP6guNmmQH828Nr8fRXDxcwrpLinwwS8KBhyxA7KU= +github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index cb85edb8..396f6325 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -586,7 +586,7 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st // marked failed, no longer live; the producer's repeated discard, without a // session, removes it. Caller holds ss.mu and has set ss.done. func (h *Handler) dropSession(ss *streamSession) error { - slog.Info("stream: mission discarded", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) + slog.Info("stream: discarding mission", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) ctx := context.Background() // The session's own row goes last: after a failure it is still there to // be marked failed. diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index 759aa027..a9dd77b4 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -80,7 +80,8 @@ func TestStreamDiscard_LostAckResolvedByResume(t *testing.T) { c2 := rh.dial() sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-lost", ProducerID: "p-1"}) - readUntil(t, c2, isType(streaming.ServerResumeCompleted)) + reply := readUntil(t, c2, isType(streaming.ServerResumeCompleted)) + assert.Equal(t, uint8(streaming.ProtocolVersionDiscard), reply.ProtocolVersion) assert.Empty(t, rh.ops()) } @@ -96,6 +97,7 @@ func TestStreamDiscard_ResumedSession(t *testing.T) { c2 := rh.dial() sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-res", ProducerID: "p-1"}) reply := readUntil(t, c2, isType(streaming.ServerResumeOK)) + assert.Equal(t, uint8(streaming.ProtocolVersionDiscard), reply.ProtocolVersion, "the producer learns the server still takes discard_mission") sendEnv(t, c2, streaming.TypeDiscardMission, reply.Seq+1, nil) readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index 009860c6..c2f4e69a 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -395,6 +395,7 @@ func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { rh := newResumeHarness(t, time.Hour) live := rh.dial() startResumable(t, live, "Live", "m-live", "p-1") + rh.waitSeq("m-live", sendStates(t, live, 2, 1, 3)) reg := rh.h.streamRegistry() running := func(missionID string) bool { @@ -422,6 +423,13 @@ func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { assert.Never(t, func() bool { return !running("m-spooled") }, 300*time.Millisecond, 10*time.Millisecond, "the resend is not superseded") sendEnv(t, c, streaming.TypeEndMission, 2, streaming.EndMissionPayload{Interrupted: true}) readUntil(t, c, isAckFor(streaming.TypeEndMission)) + + tags := map[string]string{} + for _, op := range rh.ops() { + tags[op.MissionName] = op.Tag + } + assert.Equal(t, "TvT,partial", tags["Live"], "the superseded live mission is saved as partial") + assert.Equal(t, "TvT,partial", tags["Spooled"], "the resend is saved through its interrupted end") } func TestStreamResume_RepeatedStartReusesSession(t *testing.T) { From 41b2c8108e220e7160a642721149a9e31f60d680 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:26:57 +0200 Subject: [PATCH 10/14] fix(stream): exempt a session recovery resumed from supersede Seventh review round: - A resume marked "recovered" (the extension's spool recovery) makes the session it continues exempt from its producer's next start, like a recovered start. The flag is atomic: the registry reads it unlocked. - Log "stream: mission discarded" after a successful session discard; a duplicate discard seq no longer moves lastSeq back. - Docs: a legacy session's failed discard stays failed; the recovered resume. Bump the extension to OCAP2/extension#217's latest commit. --- docs/streaming-architecture.md | 6 ++-- go.mod | 2 +- go.sum | 4 +-- internal/server/handler_stream.go | 14 +++++++-- internal/server/handler_stream_resume_test.go | 29 +++++++++++++++++++ internal/server/stream_registry.go | 4 +-- 6 files changed, 49 insertions(+), 10 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index ea42bea1..070461fe 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -100,7 +100,8 @@ the next section. waiting for the grace period. A start marked `"recovered": true` resends an older mission from the extension's spool. It supersedes nothing, and the producer's next start does not supersede it either: the producer may be - recording its next mission meanwhile. + recording its next mission meanwhile. A `resume` marked the same way makes + the session it continues exempt too. - A session not resumed within the grace period is saved with `partial` appended to its tags. A session superseded or expired before it received anything after `start_mission` is removed instead: an empty recording is not kept. @@ -174,7 +175,8 @@ names the mission (`{"missionId":"…","producerId":"…"}`): then progress acknowledgements stop short of its number. If the removal fails, the server closes the connection without acknowledging it, marks the session's row `failed` (it is removed last), and the producer repeats the - discard. + discard. A legacy session has no mission id to repeat it by; its row stays + `failed`. - The mission id is remembered as finished for an hour, in memory only, so a producer whose acknowledgement was lost gets `resume_completed`. After a web restart it gets `resume_unknown` and repeats the discard, which finds nothing diff --git a/go.mod b/go.mod index 33321e42..17cc6c5b 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a + github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 8422dc71..469fb9da 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a h1:F8rP6guNmmQH828Nr8fRXDxcwrpLinwwS8KBhyxA7KU= -github.com/OCAP2/extension/v5 v5.0.2-0.20261026235903-3c4fd911374a/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd h1:65K6wDq2UmvyFv0Ep0oQy3SzPu4uulgdeKCinKBtEys= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 396f6325..6bb1d32a 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -352,7 +352,7 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s if resumable { ss.missionID = payload.MissionID ss.producer = payload.ProducerID - ss.recovered = payload.Recovered + ss.recovered.Store(payload.Recovered) if err := h.repoOperation.SetStreamMission(context.Background(), op.ID, ss.missionID); err != nil { slog.Warn("stream: cannot record mission id", "error", err) } @@ -394,8 +394,13 @@ func (h *Handler) resumeStream(raw json.RawMessage, conn *streamConn) (streaming } reg := h.streamRegistry() if ss := reg.get(p.MissionID); ss != nil && ss.producer == p.ProducerID && h.takeOver(ss, conn) { + if p.Recovered { + // Continued from the producer's spool: not the mission it + // records live, which its next start supersedes. + ss.recovered.Store(true) + } seq := ss.lastSeq.Load() - slog.Info("stream: mission resumed", "missionId", p.MissionID, "seq", seq) + slog.Info("stream: mission resumed", "missionId", p.MissionID, "seq", seq, "recovered", p.Recovered) return reply(streaming.ServerResumeOK, seq), ss } if reg.isCompleted(p.MissionID) || h.streamEnded(p.MissionID) { @@ -575,9 +580,10 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st if h.dropSession(ss) != nil { return endFailed } - if envelope.Seq > 0 { + if envelope.Seq > ss.lastSeq.Load() { ss.lastSeq.Store(envelope.Seq) } + slog.Info("stream: mission discarded", "missionId", ss.missionID) return endSaved } @@ -634,6 +640,8 @@ func (h *Handler) discardStreamed(raw json.RawMessage) bool { // Reserving the mission id holds off a start of it while its recordings // go. A session of the mission is dropped in its place, unless a // connection still feeds it. + // A finalized session leaves the registry before it unlocks, so one retry + // is normally enough; the bound only guards against a broken invariant. for range 3 { existing, err := reg.reserve(p.MissionID, streamReserveWait) if err != nil { diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index c2f4e69a..631e860a 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -432,6 +432,35 @@ func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { assert.Equal(t, "TvT,partial", tags["Spooled"], "the resend is saved through its interrupted end") } +func TestStreamResume_RecoveredResumeIsNotSuperseded(t *testing.T) { + rh := newResumeHarness(t, time.Hour) + c := rh.dial() + startResumable(t, c, "Left", "m-left", "p-1") + rh.waitSeq("m-left", sendStates(t, c, 2, 1, 3)) + c.Close() + + // The producer's recovery continues the mission from its spool... + rec := rh.dial() + sendEnv(t, rec, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-left", ProducerID: "p-1", Recovered: true}) + readUntil(t, rec, isType(streaming.ServerResumeOK)) + + // ...while it starts recording its next mission. + next := rh.dial() + startResumable(t, next, "Next", "m-next-r", "p-1") + reg := rh.h.streamRegistry() + assert.Never(t, func() bool { + ss := reg.get("m-left") + if ss == nil { + return true + } + ss.mu.Lock() + defer ss.mu.Unlock() + return ss.done + }, 300*time.Millisecond, 10*time.Millisecond, "the resumed recovery is not superseded") + sendEnv(t, rec, streaming.TypeEndMission, 5, streaming.EndMissionPayload{Interrupted: true}) + readUntil(t, rec, isAckFor(streaming.TypeEndMission)) +} + func TestStreamResume_RepeatedStartReusesSession(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index c608c115..6e3fe918 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -58,7 +58,7 @@ type streamSession struct { mu sync.Mutex missionID string // empty for legacy sessions producer string - recovered bool // started by a resend from the producer's spool + recovered atomic.Bool // started or resumed by a resend from the producer's spool session *ingestion.Session outputDir string opID int64 @@ -200,7 +200,7 @@ func (r *streamRegistry) byProducer(producer string, keep string) []*streamSessi defer r.mu.Unlock() var out []*streamSession for id, s := range r.sessions { - if id != keep && s.producer == producer && !s.recovered { + if id != keep && s.producer == producer && !s.recovered.Load() { out = append(out, s) } } From 8277bdc6a4f6d5414edd4ca893b51b36dafe0e56 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 03:04:48 +0200 Subject: [PATCH 11/14] fix(stream): drop an interrupted end of an empty mission, mark repeated recovered starts Eighth review round: - end_mission marked interrupted on a session that received nothing after its start (recovery after a crash right after the start) removes the session instead of saving an empty partial, like grace expiry and supersede; the mission id is remembered as finished. - A recovered start that takes over an existing session marks it recovered, like a recovered resume. - dropSession's comment and the docs say how a failed or repeated discard arrives. Bump the extension to OCAP2/extension#217's latest commit. --- docs/streaming-architecture.md | 7 ++- go.mod | 2 +- go.sum | 4 +- internal/server/handler_stream.go | 49 ++++++++++++++----- internal/server/handler_stream_resume_test.go | 21 +++++++- 5 files changed, 65 insertions(+), 18 deletions(-) diff --git a/docs/streaming-architecture.md b/docs/streaming-architecture.md index 070461fe..433d08be 100644 --- a/docs/streaming-architecture.md +++ b/docs/streaming-architecture.md @@ -104,7 +104,8 @@ the next section. the session it continues exempt too. - A session not resumed within the grace period is saved with `partial` appended to its tags. A session superseded or expired before it received anything - after `start_mission` is removed instead: an empty recording is not kept. + after `start_mission`, or ended as interrupted with nothing in it, is + removed instead: an empty recording is not kept. | Setting | Default | Meaning | |---|---|---| @@ -180,7 +181,9 @@ names the mission (`{"missionId":"…","producerId":"…"}`): - The mission id is remembered as finished for an hour, in memory only, so a producer whose acknowledgement was lost gets `resume_completed`. After a web restart it gets `resume_unknown` and repeats the discard, which finds nothing - left to remove. + left to remove. A live connection repeats it by resending the mission from + its spool into a new session first, which the discard then drops with the + rest; recovery repeats it without a session. - An unconfirmed discard stays in the extension's spool. Recovery sends only the `discard_mission` again, never the mission data. When the server no longer holds the session (`resume_unknown`), for example because the resume grace diff --git a/go.mod b/go.mod index 17cc6c5b..f13a22d1 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd + github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 469fb9da..96c29f1d 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd h1:65K6wDq2UmvyFv0Ep0oQy3SzPu4uulgdeKCinKBtEys= -github.com/OCAP2/extension/v5 v5.0.2-0.20261027002426-4d419f2ad4fd/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d h1:UoGhz8JqWnEGnknx/8NMpEF0mbLT+1sK4C9XLJHG9n4= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 6bb1d32a..59214f4d 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -287,6 +287,9 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s if !h.takeOver(existing, conn) { return nil, "mission already saved" } + if payload.Recovered { + existing.recovered.Store(true) + } slog.Info("stream: mission start repeated", "missionId", payload.MissionID) return existing, "" } @@ -515,6 +518,17 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream if ss.gap || interrupted { tag = withPartialTag(tag) } + if interrupted && len(ss.counts) == 0 && ss.resumable() { + // A mission recovered after a crash right after its start: an empty + // recording is not kept, as on grace expiry. + slog.Info("stream: dropping interrupted mission that recorded nothing", "missionId", ss.missionID) + ss.done = true + h.dropEmptySession(ss, true) + if envelope.Seq > ss.lastSeq.Load() { + ss.lastSeq.Store(envelope.Seq) + } + return endSaved + } slog.Info("stream: mission ended", "missionId", ss.missionID, "counts", ss.counts) ss.done = true @@ -587,10 +601,29 @@ func (h *Handler) discardStream(ss *streamSession, conn *streamConn, envelope st return endSaved } +// dropEmptySession removes a session that received nothing after its start +// instead of saving an empty recording, and releases it; finished remembers +// the mission id as finished for resume. Caller holds ss.mu and has set +// ss.done. +func (h *Handler) dropEmptySession(ss *streamSession, finished bool) { + ctx := context.Background() + if err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)); err != nil { + slog.Error("stream: cannot remove empty mission", "missionId", ss.missionID, "id", ss.opID, "error", err) + if ss.opID > 0 { + h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) + } + } + h.streamRegistry().release() + if ss.resumable() { + h.streamRegistry().remove(ss, finished) + } +} + // dropSession removes a discarded session's recording and earlier unended // segments of its mission, and releases the session. On failure the row is -// marked failed, no longer live; the producer's repeated discard, without a -// session, removes it. Caller holds ss.mu and has set ss.done. +// marked failed, no longer live, and the producer repeats the discard: on a +// new session after resume_unknown, or without one from its spool's +// recovery. Either removes it. Caller holds ss.mu and has set ss.done. func (h *Handler) dropSession(ss *streamSession) error { slog.Info("stream: discarding mission", "missionId", ss.missionID, "mission", ss.session.Mission().MissionName, "counts", ss.counts) ctx := context.Background() @@ -803,17 +836,9 @@ func (h *Handler) expireStream(ss *streamSession, gen uint64) { ss.done = true if len(ss.counts) == 0 { // Nothing was received after the start, for example a start the - // producer gave up waiting for: an empty recording is not kept. + // producer gave up waiting for. slog.Info("stream: dropping abandoned mission that recorded nothing", "missionId", ss.missionID, "reason", reason) - ctx := context.Background() - if err := h.removeStreamOperation(ctx, ss.opID, filepath.Base(ss.outputDir)); err != nil { - slog.Error("stream: cannot remove empty mission", "missionId", ss.missionID, "id", ss.opID, "error", err) - if ss.opID > 0 { - h.repoOperation.UpdateConversionStatus(ctx, ss.opID, ConversionStatusFailed) - } - } - h.streamRegistry().release() - h.streamRegistry().remove(ss, false) + h.dropEmptySession(ss, false) return } slog.Warn("stream: saving abandoned mission as partial", "missionId", ss.missionID, "reason", reason) diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index 631e860a..d70cf64a 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -391,6 +391,24 @@ func TestStreamResume_AbandonedEmptyMissionIsDropped(t *testing.T) { assert.Equal(t, 1, reg.activeCount()) } +func TestStreamResume_InterruptedEmptyMissionIsDropped(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + // Recovery after a crash right after the start: nothing but the start. + start := startPayload("Crashed early", "m-crash-empty", "p-1") + start["recovered"] = true + sendEnv(t, c, streaming.TypeStartMission, 1, start) + readUntil(t, c, isAckFor(streaming.TypeStartMission)) + sendEnv(t, c, streaming.TypeEndMission, 2, streaming.EndMissionPayload{Interrupted: true}) + ack := readUntil(t, c, isAckFor(streaming.TypeEndMission)) + assert.Equal(t, uint64(2), ack.Seq) + + assert.Empty(t, rh.ops(), "an empty recording is not kept") + reg := rh.h.streamRegistry() + assert.True(t, reg.isCompleted("m-crash-empty"), "a lost ack is answered by resume_completed") + assert.Zero(t, reg.activeCount()) +} + func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { rh := newResumeHarness(t, time.Hour) live := rh.dial() @@ -421,7 +439,8 @@ func TestStreamResume_RecoveredStartLeavesLiveMission(t *testing.T) { startResumable(t, next, "Next", "m-next", "p-1") require.Eventually(t, func() bool { return reg.get("m-live") == nil }, 5*time.Second, 10*time.Millisecond) assert.Never(t, func() bool { return !running("m-spooled") }, 300*time.Millisecond, 10*time.Millisecond, "the resend is not superseded") - sendEnv(t, c, streaming.TypeEndMission, 2, streaming.EndMissionPayload{Interrupted: true}) + last := sendStates(t, c, 2, 1, 2) + sendEnv(t, c, streaming.TypeEndMission, last+1, streaming.EndMissionPayload{Interrupted: true}) readUntil(t, c, isAckFor(streaming.TypeEndMission)) tags := map[string]string{} From 99a9b88771b6f6266dcf6806a11c26b39d482c19 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 03:27:16 +0200 Subject: [PATCH 12/14] fix(stream): acknowledge an end only once it is marked ended, keep a recovery resumed after a supersede chose it A save whose ended marker cannot be written is not acknowledged: a discard of the mission id would take it for unended. A recovered resume sets its flag under the session lock, and a supersede scheduled before it rechecks the flag there. --- internal/server/handler_stream.go | 43 +++++++++++++------ .../server/handler_stream_discard_test.go | 36 ++++++++++++++++ internal/server/handler_stream_resume_test.go | 31 +++++++++++++ internal/server/stream_registry.go | 3 ++ 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/internal/server/handler_stream.go b/internal/server/handler_stream.go index 59214f4d..a0eb1d42 100644 --- a/internal/server/handler_stream.go +++ b/internal/server/handler_stream.go @@ -284,12 +284,9 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s return nil, "mission id in use" } // The producer repeats a start whose acknowledgement it missed. - if !h.takeOver(existing, conn) { + if !h.takeOver(existing, conn, payload.Recovered) { return nil, "mission already saved" } - if payload.Recovered { - existing.recovered.Store(true) - } slog.Info("stream: mission start repeated", "missionId", payload.MissionID) return existing, "" } @@ -367,12 +364,19 @@ func (h *Handler) startStream(envelope streaming.Envelope, conn *streamConn) (*s // takeOver makes conn the session's connection, closing any previous one and // cancelling a pending grace expiry. It fails once the session is saved. -func (h *Handler) takeOver(ss *streamSession, conn *streamConn) bool { +// recovered marks the session continued from the producer's spool: not the +// mission it records live, which its next start supersedes. It is set under +// the session's lock, so a supersede already scheduled sees it (see +// expireStream). +func (h *Handler) takeOver(ss *streamSession, conn *streamConn, recovered bool) bool { ss.mu.Lock() defer ss.mu.Unlock() if ss.done { return false } + if recovered { + ss.recovered.Store(true) + } if ss.owner != nil && ss.owner != conn { ss.owner.kick() // a half-open predecessor must not feed the session any more } @@ -396,12 +400,7 @@ func (h *Handler) resumeStream(raw json.RawMessage, conn *streamConn) (streaming return reply(streaming.ServerResumeUnknown, 0), nil } reg := h.streamRegistry() - if ss := reg.get(p.MissionID); ss != nil && ss.producer == p.ProducerID && h.takeOver(ss, conn) { - if p.Recovered { - // Continued from the producer's spool: not the mission it - // records live, which its next start supersedes. - ss.recovered.Store(true) - } + if ss := reg.get(p.MissionID); ss != nil && ss.producer == p.ProducerID && h.takeOver(ss, conn, p.Recovered) { seq := ss.lastSeq.Load() slog.Info("stream: mission resumed", "missionId", p.MissionID, "seq", seq, "recovered", p.Recovered) return reply(streaming.ServerResumeOK, seq), ss @@ -537,8 +536,10 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream if err == nil && ss.resumable() { // Marked before the session leaves the registry, so a discard of the // mission id never takes the saved recording for an unended one. - if err := h.repoOperation.MarkStreamEnded(ctx, ss.opID); err != nil { - // A discard of the mission id would now take it for unended. + // Without the mark the save does not count: end_mission is not + // acknowledged, and the producer resends the mission, whose + // recording replaces this one, or discards it. + if err = h.markStreamEnded(ctx, ss.opID); err != nil { slog.Error("stream: cannot mark mission ended", "missionId", ss.missionID, "id", ss.opID, "error", err) } } @@ -557,6 +558,17 @@ func (h *Handler) endStream(ss *streamSession, conn *streamConn, envelope stream return endSaved } +// markStreamEnded records that the session's row was saved through +// end_mission. +func (h *Handler) markStreamEnded(ctx context.Context, id int64) error { + if hook := streamTestHooks.beforeMarkEnded; hook != nil { + if err := hook(); err != nil { + return err + } + } + return h.repoOperation.MarkStreamEnded(ctx, id) +} + // replaceStreamSegments removes the partial recordings an earlier attempt of // the mission left behind, once a complete recording of it is saved. They // exist when the producer resent the mission in full, for example after a web @@ -821,6 +833,11 @@ func (h *Handler) expireStream(ss *streamSession, gen uint64) { if gen != 0 && (ss.owner != nil || gen != ss.detachGen) { return // resumed, or detached again with a newer timer } + if gen == 0 && ss.recovered.Load() { + // Recovery resumed it after the new start chose it: a resend from + // the spool is not superseded (see byProducer). + return + } if ss.owner != nil { ss.owner.kick() ss.owner = nil diff --git a/internal/server/handler_stream_discard_test.go b/internal/server/handler_stream_discard_test.go index a9dd77b4..a07d72ce 100644 --- a/internal/server/handler_stream_discard_test.go +++ b/internal/server/handler_stream_discard_test.go @@ -459,6 +459,42 @@ func TestStreamDiscard_WithoutSessionKeepsRecordingSavedMeanwhile(t *testing.T) require.Len(t, rh.ops(), 1, "a recording saved through end_mission is never discarded") } +func TestStreamDiscard_EndNotAcknowledgedWithoutEndedMark(t *testing.T) { + rh := newResumeHarness(t, time.Minute) + c := rh.dial() + startResumable(t, c, "Unmarked", "m-unmarked", "p-1") + last := sendStates(t, c, 2, 1, 3) + rh.waitSeq("m-unmarked", last) + + // The recording is saved, but marking it ended fails: the discard + // protection is not durable, so the save does not count. + streamTestHooks.beforeMarkEnded = func() error { return errors.New("database is locked") } + t.Cleanup(func() { streamTestHooks.beforeMarkEnded = nil }) + sendEnv(t, c, streaming.TypeEndMission, last+1, nil) + c.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + var m streaming.AckMessage + err := c.ReadJSON(&m) + if err != nil { + require.True(t, websocket.IsCloseError(err, websocket.CloseInternalServerErr), "%v", err) + break + } + require.NotEqual(t, streaming.TypeEndMission, m.For, "end_mission is not acknowledged without the mark") + require.Less(t, m.Seq, last+1) + } + assert.False(t, rh.h.streamRegistry().isCompleted("m-unmarked")) + streamTestHooks.beforeMarkEnded = nil + + // The producer was never told the recording is kept: resume finds it + // unknown, and the producer's discard removes it. + c2 := rh.dial() + sendEnv(t, c2, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-unmarked", ProducerID: "p-1"}) + readUntil(t, c2, isType(streaming.ServerResumeUnknown)) + sendEnv(t, c2, streaming.TypeDiscardMission, 0, discardPayload("m-unmarked", "p-1")) + readUntil(t, c2, isAckFor(streaming.TypeDiscardMission)) + assert.Empty(t, rh.ops()) +} + func TestStreamDiscard_WithoutSessionHoldsOffStart(t *testing.T) { rh := newResumeHarness(t, 100*time.Millisecond) c := rh.dial() diff --git a/internal/server/handler_stream_resume_test.go b/internal/server/handler_stream_resume_test.go index d70cf64a..b568c507 100644 --- a/internal/server/handler_stream_resume_test.go +++ b/internal/server/handler_stream_resume_test.go @@ -480,6 +480,37 @@ func TestStreamResume_RecoveredResumeIsNotSuperseded(t *testing.T) { readUntil(t, rec, isAckFor(streaming.TypeEndMission)) } +func TestStreamResume_RecoveredResumeAfterSupersedeChoseIt(t *testing.T) { + rh := newResumeHarness(t, time.Hour) + c := rh.dial() + startResumable(t, c, "Left", "m-chosen", "p-1") + rh.waitSeq("m-chosen", sendStates(t, c, 2, 1, 3)) + c.Close() + rh.detachedSession("m-chosen") + + // The producer's next start chooses the waiting mission to supersede... + reg := rh.h.streamRegistry() + chosen := reg.byProducer("p-1", "m-next-c") + require.Len(t, chosen, 1) + + // ...its recovery resumes the mission before the supersede runs... + rec := rh.dial() + sendEnv(t, rec, streaming.TypeResume, 0, streaming.ResumePayload{MissionID: "m-chosen", ProducerID: "p-1", Recovered: true}) + readUntil(t, rec, isType(streaming.ServerResumeOK)) + + // ...and then the supersede runs. + rh.h.expireStream(chosen[0], 0) + ss := reg.get("m-chosen") + require.NotNil(t, ss, "the resumed recovery stays registered") + ss.mu.Lock() + done, owned := ss.done, ss.owner != nil + ss.mu.Unlock() + assert.False(t, done, "the resumed recovery is not saved") + assert.True(t, owned, "its connection is not closed") + sendEnv(t, rec, streaming.TypeEndMission, 5, streaming.EndMissionPayload{Interrupted: true}) + readUntil(t, rec, isAckFor(streaming.TypeEndMission)) +} + func TestStreamResume_RepeatedStartReusesSession(t *testing.T) { rh := newResumeHarness(t, time.Minute) c := rh.dial() diff --git a/internal/server/stream_registry.go b/internal/server/stream_registry.go index 6e3fe918..fb5baf0e 100644 --- a/internal/server/stream_registry.go +++ b/internal/server/stream_registry.go @@ -30,6 +30,9 @@ var streamTestHooks struct { // beforeRemove runs before a streamed recording is removed; an error // stands in for a failed removal. beforeRemove func() error + // beforeMarkEnded runs before a saved recording is marked ended; an + // error stands in for a failed write. + beforeMarkEnded func() error // beforeDiscardSession runs before a session-less discard_mission // handles a session of its mission. beforeDiscardSession func() From 02700358a78490f9ed75cae11e38037944cf4ee7 Mon Sep 17 00:00:00 2001 From: Smith <13200576+smitt14ua@users.noreply.github.com> Date: Tue, 27 Oct 2026 03:40:43 +0200 Subject: [PATCH 13/14] chore(deps): bump OCAP2/extension to the latest commit of OCAP2/extension#217 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index f13a22d1..d82fcbe2 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d + github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892 github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 96c29f1d..27dfab92 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d h1:UoGhz8JqWnEGnknx/8NMpEF0mbLT+1sK4C9XLJHG9n4= -github.com/OCAP2/extension/v5 v5.0.2-0.20261027010148-9cac5c36cc3d/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892 h1:Adl0GAaAj5Eg3up22xmvz1m7v3i7hOpYpwdBMeogT4I= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= From 2a42bf91082133cb819f6c578f6c678bf90684e4 Mon Sep 17 00:00:00 2001 From: Florian Kinder Date: Tue, 27 Oct 2026 10:40:49 +0100 Subject: [PATCH 14/14] chore(deps): pin extension to the merge commit of OCAP2/extension#217 --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index d82fcbe2..2d9a7fea 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/OCAP2/web go 1.27.2 require ( - github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892 + github.com/OCAP2/extension/v5 v5.0.2-0.20261027093953-0a7ce6a9b9a7 github.com/getkin/kin-openapi v0.151.0 github.com/go-fuego/fuego v0.21.0 github.com/golang-jwt/jwt/v5 v5.3.1 diff --git a/go.sum b/go.sum index 27dfab92..5a396de7 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892 h1:Adl0GAaAj5Eg3up22xmvz1m7v3i7hOpYpwdBMeogT4I= github.com/OCAP2/extension/v5 v5.0.2-0.20261027013159-6e1878442892/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027093953-0a7ce6a9b9a7 h1:s4z0SDRxEJo9zNVTD6UaQvgEBYv5R8kq/LErQZztjTY= +github.com/OCAP2/extension/v5 v5.0.2-0.20261027093953-0a7ce6a9b9a7/go.mod h1:+sonvAIzk+dRa7sFzKfjpcKcCFpDo7wyQF6M63XxUsg= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8=