[I] [BanyanDB] Stream writes acknowledged in the last flush interval before a graceful shutdown are lost [skywalking]

["tristaZero (via GitHub)" <gi...@apache.org>]

tristaZero opened a new issue, #14101:
URL: https://github.com/apache/skywalking/issues/14101

   ### Summary
   
   BanyanDB acknowledges a stream write with `STATUS_SUCCEED` as soon as the write is in a memory part. If a graceful shutdown (SIGTERM) comes within the next flush interval (1 s by default), the write is not on disk after the restart. The close path of the table stops the flusher without flushing the memory parts it holds.
   
   We found this while writing the integration tests requested for native self-stored logs (apache/skywalking-banyandb#1368). One of the requested cases — events admitted before graceful shutdown are retrievable after restart — cannot pass until this is fixed.
   
   ### Environment
   
   - BanyanDB standalone, built from `feat/self-stored-logs-phase1` (apache/skywalking-banyandb#1368). The files involved (`banyand/stream/flusher.go`, `tstable.go`, `stream.go`, `write_standalone.go`) are identical to apache/skywalking-banyandb `main` at [`89403f9`](https://github.com/apache/skywalking-banyandb/commit/89403f9211cbd9c56c37f37a1526610591cb1c55).
   - Default `--stream-flush-timeout` (1 s).
   - Reproduced on macOS. The code path does not depend on the platform.
   
   ### How to reproduce
   
   1. Start a standalone node.
   2. Create a stream group and a stream, for example group `ugprobe` with stream `usprobe`, entity `svc`, and tags `svc` and `msg`.
   3. Write element A with `StreamService.Write`. Wait 4 s.
   4. Write element B. Wait 0.3 s.
   5. Send SIGTERM and wait for the process to exit.
   6. Restart the node and query `msg = A` and `msg = B`.
   
   | Element | Written before SIGTERM | Write status | After restart |
   |---|---|---|---|
   | A | 4 s | `STATUS_SUCCEED` | found |
   | B | 0.3 s | `STATUS_SUCCEED` | **missing** |
   
   Before the shutdown, a query for A returned it, so the write and the query both work. Only the persistence of B fails.
   
   <details>
   <summary>Writer used for steps 3 and 4</summary>
   
   ```go
   wc, _ := streamv1.NewStreamServiceClient(conn).Write(ctx)
   _ = wc.Send(&streamv1.WriteRequest{
   	Metadata:  &commonv1.Metadata{Name: "usprobe", Group: "ugprobe"},
   	MessageId: uint64(time.Now().UnixNano()),
   	Element: &streamv1.ElementValue{
   		ElementId: nonce,
   		Timestamp: timestamppb.New(time.Now().Truncate(time.Millisecond)),
   		TagFamilies: []*modelv1.TagFamilyForWrite{{Tags: []*modelv1.TagValue{str("svc-a"), str(nonce)}}},
   	},
   })
   _ = wc.CloseSend()
   resp, _ := wc.Recv() // resp.GetStatus() == "STATUS_SUCCEED" for both A and B
   ```
   
   </details>
   
   ### Cause
   
   References are to `main` at `89403f9`.
   
   - The write callback acknowledges the write after the elements are in a memory part ([`write_standalone.go#L339`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/write_standalone.go#L339)). Nothing is on disk yet.
   - The flusher waits up to `flushTimeout` (1 s by default, [`stream.go#L47`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/stream.go#L47)) for memory parts to accumulate ([`flusher.go#L121`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/flusher.go#L121)). When the table closes, the flusher loop returns without flushing ([`flusher.go#L38-L39`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/flusher.go#L38-L39)).
   - `tsTable.Close()` stops the loops and releases the snapshot. It does not flush the memory parts ([`tstable.go#L342`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/tstable.go#L342)).
   - A memory part added while the table closes is released, and the function returns without an error ([`tstable.go#L394-L398`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/stream/tstable.go#L394-L398)).
   
   So a write acknowledged in the last `flushTimeout` before a graceful shutdown is lost, and the client is not told.
   
   `banyand/measure/flusher.go` and `banyand/trace/flusher.go` have the same close branch at lines 38–39. We did not verify data loss for measure or trace.
   
   ### Impact on the native-log integration tests (apache/skywalking-banyandb#1368)
   
   The review of #1368 asks the tests to verify that *"startup events and events admitted before graceful shutdown are retrievable after the appropriate flush/restart."* Our case TC4b covers the second half:
   
   1. Start a producer whose native flush cannot fire (`--logging-native-flush-interval=1h --logging-native-flush-size=100000`), so that only the shutdown drain can write.
   2. Log a unique line, send SIGTERM, restart, and query for the line.
   
   At shutdown, the native-log service stops before the stream service and drains its buffer into the stream. The drain publishes successfully: we traced `err=nil` and an empty per-node error map. But the drained events are in memory parts, and the stream closes within the same flush interval, so it discards them. The drain always runs inside that window, so TC4b cannot pass on standalone or data nodes, whatever the native-log code does.
   
   The other requested cases are not affected:
   
   | Case | Status |
   |---|---|
   | TC1 persisted and queryable | not affected |
   | TC2 per-node attribution | not affected |
   | TC3a/b independent thresholds, disabled by default | not affected |
   | TC4a startup events | not affected |
   | TC4b events admitted before graceful shutdown | **blocked by this issue** |
   | TC5 drop accounting and recovery | not affected: its recovery event is written well after the restart |
   
   We verified that native log rows written well before a shutdown are still there after a restart.
   
   ### Proposed solution
   
   A separate PR, in the storage layer:
   
   1. On close, flush every memory part to disk before the loops stop, with a time limit so that shutdown cannot hang. An acknowledged write is then on disk after a graceful shutdown.
   2. When a write arrives after close has started, report the failure to the caller instead of releasing the part silently, so that the client does not get `STATUS_SUCCEED` for data that is not stored.
   3. Apply the same change to measure and trace if they share this path.
   4. Add an integration test: write, send SIGTERM at once, restart, and query.
   
   Until this lands, #1368 keeps TC4b as a pending test that references this issue. The other cases run in CI.
   
   Alternatives we considered:
   
   - **Keep the native-log service running for one flush interval after its drain.** This works only for native logs, does not help user data, and slows every shutdown. We have not verified that it is sufficient.
   - **Remove TC4b.** This leaves the review item unmet.
   
   ### Questions
   
   1. Is flushing on close acceptable for shutdown time, and which time limit do you prefer?
   2. A write during close should fail. Which status should it use? A `*common.Error` with `STATUS_INTERNAL_ERROR` makes the liaison fail over the data node ([`queue/pub/client.go#L179-L185`](https://github.com/apache/skywalking-banyandb/blob/89403f9211cbd9c56c37f37a1526610591cb1c55/banyand/queue/pub/client.go#L179-L185)), so a different status is probably needed.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [BanyanDB] Stream writes acknowledged in the last flush interval before a graceful shutdown are lost [skywalking]

["tristaZero (via GitHub)" <gi...@apache.org>]

tristaZero commented on issue #14101:
URL: https://github.com/apache/skywalking/issues/14101#issuecomment-5792004787

   Following the maintainer's position that BanyanDB does not promise that acknowledged writes survive a graceful shutdown, no test depends on this any more: the case that required it has been removed from apache/skywalking-banyandb#1368 and replaced by a smoke case. The behaviour described here is unchanged and still reproducible; closing as it is not a goal of the design. The remaining question, whether a write that will not be persisted should still be acknowledged with `STATUS_SUCCEED`, can be reopened separately if wanted.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [BanyanDB] Stream writes acknowledged in the last flush interval before a graceful shutdown are lost [skywalking]

["hanahmily (via GitHub)" <gi...@apache.org>]

hanahmily commented on issue #14101:
URL: https://github.com/apache/skywalking/issues/14101#issuecomment-5789073518

   Closing this issue because the reported loss of acknowledged but unflushed writes does not establish a violation of BanyanDB's existing durability contract. Write acknowledgment does not guarantee durable persistence, and the native-log shutdown drain submits buffered events to the normal stream write path; it does not guarantee that those events survive an immediate restart.
   
   TC4b is therefore not required for apache/skywalking-banyandb#1368 and should be removed rather than left pending on this issue. My earlier review request was too broad on this point. Coverage should verify stopping admission and draining the native-log buffer during graceful shutdown, without introducing a new storage persistence guarantee.
   
   Flush-on-close can be discussed separately as an enhancement. The potential false-success response for writes rejected during close is also a separate concern and would need a focused reproduction of a reachable shutdown race. Neither should block the native self-stored logs PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [BanyanDB] Stream writes acknowledged in the last flush interval before a graceful shutdown are lost [skywalking]

["hanahmily (via GitHub)" <gi...@apache.org>]

hanahmily closed issue #14101: [BanyanDB] Stream writes acknowledged in the last flush interval before a graceful shutdown are lost
URL: https://github.com/apache/skywalking/issues/14101


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org