Skip to content

fix: LatestFrameWriter reclaims a stale segment whose pid was reused - #5

Open
rafalmaciag wants to merge 1 commit into
masterfrom
fix/latest-frame-stale-start-time
Open

rafalmaciag wants to merge 1 commit into
masterfrom
fix/latest-frame-stale-start-time

Conversation

@rafalmaciag

Copy link
Copy Markdown

Problem (observed, epic-144, 2026-11-07)

After the rocket-welder container was recreated, a camera pipeline failed to start with not-negotiated. A shared /dev/shm still held the segment that the killed container's displaysink wrote. The plugin logged:

ERROR zerobuffer gstdisplaysink.cpp:406:gst_display_sink_set_caps:<displaysink0> Failed to create latest-frame buffer 'x': LatestFrameWriter: segment 'x' is owned by a live writer (pid 8)

pid 8 was the new writer itself.

Root cause

  1. On Linux, platform::get_process_start_time read field 3 of /proc/<pid>/stat into a uint64_t. Field 3 is the state character (S, R), so the read failed and the function returned 0 for every process. As a result, every LatestFrameWriter wrote writer_start_time = 0 (header offset 80).
  2. reclaim_stale_or_throw reads start == 0 as "skip the pid-reuse check". Liveness then rested on kill(pid, 0) alone.
  3. A fresh container in its own pid namespace hands out the same pids again, so the new writer often gets the dead writer's pid. kill(<thread id>, 0) also succeeds: on the epic-144 env, a stale header's pid 187 was a thread of the new gst-launch. In both cases the dead segment counts as live, the constructor throws, and set_caps fails.

Fix

  • Read the fields after comm as text tokens. Field 22 is the 20th token.
  • Treat writer_start_time == 0 as stale. It cannot prove a live owner, because every Linux writer before this fix wrote 0. Without this, the first restart after upgrading still fails on the segments the old writers left.

Evidence

  • Unit tests (test_latest_frame):
    • Before the fix: 3 of the 4 new tests fail (start time is 0; a writer records 0; a stale segment with its own pid and start 0 is rejected). The 4th, reused pid with a different start time, already passed and guards against regressions.
    • After the fix: 22/22 pass, and also 22/22 under ASan+UBSan.
  • Generated (harmony) tests: the same failure list before and after. They fail on Step not found: we run in harmony and are unrelated.
  • Container repro (two containers on one tmpfs folder mounted as /dev/shm, docker rm -f the first, then start the same writer in a second container):
Run Failures
rocketwelder 2.59.0 displaysink (zerobuffer 0d795e2), same pid 10/10
rocketwelder 2.59.0 displaysink, different pid 0/10
LatestFrameWriter on master 10/10
LatestFrameWriter on this branch 0/10
Upgrade case (old writer leaves the segment, fixed writer starts) 0/5

Release path

The fix reaches rocket-welder only through streamer. streamer pins src/zerobuffer at 0d795e2 (v3.2.8), so rocket-welder needs:

  1. a streamer submodule bump and a streamer release;
  2. a bump of the streamer: base image in rw2's Dockerfiles;
  3. an rw2 release;
  4. a rocketwelder-compose bump.

The reader (native-player) does not need this change.

Not changed

platform_windows.cpp defines neither get_process_start_time nor get_current_process_start_time. This PR does not touch Windows.

🤖 Generated with Claude Code

platform::get_process_start_time (Linux) extracted field 3 of
/proc/<pid>/stat (the state character, e.g. 'S') into a uint64_t, the
extraction failed and the function returned 0 for every process. Every
writer therefore recorded writer_start_time = 0, and reclaim_stale_or_throw
treated start == 0 as "skip the pid-reuse check", so liveness was only
kill(pid, 0).

When a container holding a writer is killed without a clean close
(docker rm -f) and /dev/shm is shared with the host, the next container
reuses the same pids: the new writer found its OWN pid (or one of its
threads' ids - kill(tid, 0) succeeds) in the stale header, judged the
segment live, threw, and displaysink failed set_caps with not-negotiated.
Reproduced 10/10 with the rocketwelder 2.59.0 image.

- parse /proc/<pid>/stat fields after comm as text tokens; field 22 is
  the 20th token
- a header with writer_start_time == 0 cannot prove a live owner (every
  pre-fix Linux writer wrote 0): treat it as stale
- tests: start time equals /proc/self/stat field 22; a writer records it;
  stale segment with own pid + start 0 is reclaimed; stale segment with
  own pid + different start is reclaimed

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rafalmaciag rafalmaciag left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cpp-reviewer verdict: APPROVED (no BLOCKER/MAJOR; 3 MINOR)

What I ran (own clone /tmp/claude-1001/zb-review at 82d60b9, master worktree beside it; cpp/build.sh Debug in a throwaway Ubuntu 24.04 container rw144-review-zb)

  • Before the fix: master cpp/src with this PR's tests/test_latest_frame.cpp gives 19 passed, 3 failed:
    • ProcessStartTimeIsProcStatField22 (got 0, expected 83843191)
    • WriterRecordsRealStartTime (got 0)
    • StaleSegmentWithOwnPidAndZeroStartTimeReclaimed (throws "owned by a live writer")
    • StaleSegmentWithReusedPidReclaimed passes on master too. That is expected: the old code compared a nonzero recorded start against the broken parser's 0.
  • After the fix, built with ASan+UBSan (libasan.so.8 and libubsan.so.1 linked): test_latest_frame 22/22, with no sanitizer reports. --gtest_repeat=20 on the stale, start-time and second-writer tests: 0 failures.
  • No compiler warnings in the PR build.
  • Full ctest: the non-generated tests (1–29) all pass. The generated Harmony scenario tests (30–97) fail the same way on master and on this branch ("Step not found: we run in harmony", because they need the Harmony harness). They have nothing to do with this PR.

Checked, no issue

  • /proc/<pid>/stat parse (cpp/src/platform_linux.cpp:44-66):
    • rfind(')') is right. comm may contain spaces and ), but no field after it does, so the last ) always closes comm.
    • The loop field = 3..22 reads 20 whitespace tokens, so the last token is field 22 (starttime).
    • stoull sits inside try/catch, and any failure returns 0.
    • My own test run cross-checks the value against the test's own oracle, and the engineer's container evidence cross-checks it against /proc (8 (lf_writer_maste) S 83812919).
  • Self-pid case:
    • A dead writer whose pid is now our own pid recorded a different start tick, so the segment is judged stale.
    • A genuine second writer in the same process (same pid, same start) is still refused. SecondWriterOnLiveSegmentRejected now actually exercises the start-time comparison; before, it passed through the start == 0 shortcut.
  • Thread-id case:
    • kill(tid, 0) still succeeds, but that no longer decides anything alone. /proc/<tid>/stat exists for non-leader threads (it is just not listed), and its field 22 is the thread's own creation tick.
    • Ticks count since host boot, and a reused pid or tid is always created after the dead writer started. A false "alive" therefore needs the reused id to be created in the same 10 ms tick as the dead writer. That cannot happen when a container is recreated.
    • Without time namespaces (Docker does not use them), both sides read the same clock.
  • Performance: both /proc reads are on the create path only: the constructor's get_current_process_start_time() and the reclaim_stale_or_throw() fallback. Nothing changed in publish or read.
  • Cross-language layout: LatestFrameSharedHeader is untouched (still 128 B, static_assert intact). Only the value written to writer_start_time@80 changes, from 0 to the real tick. No C# or Python implementation reads or writes that field. The reader's liveness is magic-only (latest_frame.cpp:377-394), so readers are not affected.
  • Upgrade path: an old-writer stale header (start = 0) is reclaimed by a new writer, 0/5 failures (lib-upgrade-old-stale-new-writer-5x.txt).

Finding #1: "start 0 = stale" lets a new writer take a segment from a LIVE old-version writer

Severity: MINOR
Category: Design
Evidence: cpp/src/latest_frame.cpp:146-147, alive = pid != 0 && start != 0 && ...; release() at latest_frame.cpp:158-167 unlinks by name.

Issue: a live writer has start time 0 in two cases:

  • (a) It runs a pre-fix library, for example the gstzerobuffer.so in rocketwelder 2.59.0 (submodule 0d795e2).
  • (b) /proc is unreadable where it runs.

A new-version writer opening the same name on the same /dev/shm while one of these is alive now unlinks the name and creates its own segment. Before this PR it was refused, as long as the pids differed.

When this can happen: only when two writers with the same segment name run at the same time on a shared /dev/shm. That means an old-version container and a new-version container running the same pipeline id side by side (both are up during a manual blue/green, or two stations seeded from the same pipeline id share /dev/shm). A normal rw2 upgrade does not do this. docker compose stops the old container, which kills its gst children, before starting the new one. So the old writer is dead, and reclaiming its segment is exactly the observed fix.

Effect if it happens:

  • The old writer keeps writing to its now-unlinked mapping. Readers already attached to it stay there.
  • When the old writer later closes cleanly, its release() calls shm_unlink(name), which removes the NEW writer's name. New readers then cannot attach until the new writer is recreated.
  • This is not a memory-safety problem.

Recommendation: no code change now. Nothing observed asks for it (happy-path-first). Add one line to the epic's "Deferred until observed" list: "mixed-version concurrent writers on one segment name: new writer reclaims; old writer's close unlinks the new name." If it is ever observed, the fix is to have release() unlink only when its fd's st_ino still matches /dev/shm/<name>.

Finding #2: process_exists is now redundant in the alive test

Severity: MINOR
Category: Standards (simplicity)
Evidence: cpp/src/latest_frame.cpp:146-147

Issue: with start != 0 required, get_process_start_time(pid) == start already implies the process exists, because a missing /proc/<pid> returns 0. The kill(pid, 0) call only adds a way to be wrong: with EPERM it reports a live process owned by another uid as dead. That outcome is still harmless today, because the sticky /dev/shm blocks the unlink and the retried create() throws.

Question: drop platform::process_exists(pid) from the expression? Optional.

Finding #3: the header contract does not mention the start-0 rule

Severity: MINOR
Category: Design (docs)
Evidence: cpp/include/zerobuffer/latest_frame.h:69 (writer_start_time; // writer process start time (pid-reuse guard)) and the class comment "Reclaims a stale segment left by a dead writer on create".

Issue: the rule "0 means unknown, so the segment is treated as stale and reclaimed" lives only in a .cpp comment, which also carries history ("epic-144, 2026-11-07"). Another-language writer would need this rule from the header. One clause on the field comment would cover it. Optional.

Not in scope (stated by the engineer, confirmed)

platform_windows.cpp has no get_process_start_time / get_current_process_start_time, and SharedMemory::remove is a no-op there, so this PR changes nothing on Windows.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant