Keep telemetry out of S2 when storage is disabled

[archandatta]

An instance whose telemetry sessions ask for storage.enabled=false never opens its S2 append session, and S2 never receives an event that was captured with storage off.

BrowserTelemetryConfig gains storage: { enabled }, a sibling of export. An omitted block means true, the opposite of export, so callers that predate the field keep storing. In a PUT an omitted block means on; in a PATCH an omitted field leaves the current value unchanged. TelemetryState.config echoes it on GET, PUT and PATCH, including on the cleared config a stop request returns, which reflects the toggle that request carried. Callers can compare the echo to what they sent. That matters because the strict handler decodes with plain encoding/json: only POST /repl rejects unknown fields, so an image that predates this field accepts storage and returns 201 without honoring it.

When the sink opens

main no longer starts the controller at boot or from the fork-identity hook. reconcileStorage is deferred from PUT and PATCH the same way as reconcileExport. It opens the sink once a capture session is active with storage on, and it never closes it. The writer is single-use and binds its stream for the life of the instance, so it still stops only at shutdown. Because it can't be closed, a PUT or PATCH whose resulting config has storage off gets a 409 once the sink has ever opened, and the 409 is returned before anything is committed. A stop PUT carrying storage.enabled=false on an instance that has stored is refused the same way, since it asks for a state the instance can no longer honor. A stop PUT that omits storage still clears. A failed start is logged and retried by the next request, matching export.

Every PUT and PATCH takes storageMu before the storage check and holds it until the config is committed or rolled back. reconcileStorage holds the same lock across its read of the session and Start, so it only ever reads a settled config. That closes two races:

• Guard vs start. A storage-off config committed while a start was in flight would leave storage off configured with the sink open.
• Provisional commit vs rollback. A storage-on update commits before capture is applied. A reconcileStorage deferred from an earlier request could read that provisional config and open the sink. If capture then failed, the request would roll back to storage off with the sink open.

The lock order is always monitorMu then storageMu, and reconcileStorage never takes monitorMu. Unlike exportMu, storageMu is taken under monitorMu, so a PUT or PATCH can hold monitorMu while it waits for a Start that is running. Start does not dial, because s2-sdk-go opens the transport on first submit, so that wait is short.

Why the writer now starts after a seq

S2StorageWriter read the ring from its oldest event, which is correct when the sink opens at boot. With the start deferred to the handler, that behavior would copy events from a storage-off session into S2 when a later storage-on config opened the sink. This covers a storage-off session followed by storage-on in the same session, and a cleared storage-off session followed by a storing one. TelemetrySession now records the last seq published before storage was turned on for the current session. It reads that seq under the session mutex that Publish holds, so the boundary is exact. S2StorageController.Start and NewS2StorageWriter take it as afterSeq, and the writer reads the ring after it. The first storing session on a fresh process still stores from seq 0, since nothing reaches the ring without a session: TelemetrySession is its only publisher and drops everything while inactive. Deferring the start therefore loses nothing. A start that failed and is retried keeps the session's original floor, so events captured with storage on but not yet stored are still sent.

This changes the controller's Start signature, and S2StorageWriter.Start is split so the forwarding half can run against a mock backend. The controller tests from #401 now pass 0. New tests assert that the writer forwards exactly from afterSeq+1, and that a controller started after the ring head reads nothing.

Forks

The fork hook now only records the applied payload's S2_STREAM in-process, and the handoff no longer starts anything. While the fork-identity wait is armed and no identity has been applied, the resolver returns no stream. A storing config applied before the handoff therefore leaves the sink closed and logs a warning, where the old code would have bound the parent's stream. The next PUT or PATCH after the handoff opens the sink on the fork's stream. If no config follows the handoff, the sink stays closed until one does. A fork's boot S2_STREAM belongs to the parent, so an applied payload without s2_stream now leaves storage closed. #404 fell back to the boot value in that case. The platform always sends s2_stream as the fork's own instance name, so the fallback could only ever bind the parent's stream.

Unchanged: shutdown order and the bounded Stop, missing credentials or stream keeping the sink closed, export semantics, and the resolver's restart path through the persisted applied payload.

Testing

• go build ./..., go vet ./... — clean
• go test $(go list ./... | grep -v /e2e$) -count=1 -race — every package passes at head. An earlier run hit TestUpstreamManagerDetectsChromiumAndRestart in lib/devtoolsproxy, which this PR does not touch, on the same t.TempDir teardown race Drive the S2 storage sink from its controller #404 reported. It passed -count=3 on rerun.
• New tests cover the never-start path, a single start across repeated PUT and PATCH, the 409 on PUT, PATCH and a clearing PUT with config, seq and applied_at unchanged, echoes on every response, the start floor after storage-off capture, the retry floor, and the writer's exact first seq. They also cover a storage-off request waiting on an in-flight start, and a deferred reconcile waiting out a storage-on PUT or PATCH that rolls back. Finally, they cover no ring publish without a session, and the resolver withholding the parent stream while a fork is pending or its payload has no stream. Each guarded line was mutated and its test failed: the lock scope, an early lock release, each 409, the floor off by one in both directions, and both fork fallbacks. One check turned out redundant and was removed.
• Headless image built from this branch, API log read at /var/log/supervisord/kernel-images-api:
  • S2 unset: 0 S2 storage enabled at boot; PUT network 201 and still 0; GET echoes storage.enabled=true
  • fake S2: 0 at boot; PUT with storage omitted 201 and 1; POST /telemetry/events 200 and the event arrives on /telemetry/stream; storage-off PUT, PATCH, and clearing PUT all 409; GET afterward still true
  • fresh fake S2: storage-off PUT 201 and 0; two events published (seqs 1–2) reach the stream with still 0 lines; PUT storage omitted 200 and 1; the next event is seq 3, and S2 received only seq 3, as shown by the SDK's per-record ack errors against the fake basin
  • fork wait with seed S2_STREAM=seed-s: 0 at boot; POST /internal/fork-identity 204 and still 0; PUT 201 and 1, bound to stream=fork-s
  • fork wait, storing PUT before the handoff: 201, 0 lines, and one pending warning; after the handoff a PATCH opens it on stream=fork-s
  • fork wait, payload without s2_stream: handoff 204, then PUT 201 with 0 lines
  • docker stop -t 30 in both fake-S2 modes: exit 0, no drain warning. One s2 storage writer stop failed line appears because the fake basin's host does not resolve. The main image logs the same line under the same steps, and this image logs none when no event was sent.
• go test ./e2e/ -count=1 -timeout 110m against headless and headful images built from this branch: 52 passed, 0 failed, 2 skipped (34 subtests pass), the same as Drive the S2 storage sink from its controller #404 reported for main. The skips are TestReplayRecordingZombocomArchiveAudio, which needs a network fixture, and TestS2StorageWriter, which needs real S2 credentials. TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 was set because this machine could not resolve localhost.

Not run: real S2 credentials, which were not available. Records reaching a real stream were checked only through the fake basin's per-record ack errors.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes when and what telemetry is persisted to S2 and fork stream binding, which affects data isolation on forks and irreversible storage-on instances; extensive tests mitigate regressions.

Overview
Adds a storage.enabled toggle to telemetry (default on when omitted, unlike export) and defers opening the S2 append sink until the first capture session that wants storage, via reconcileStorage on PUT/PATCH instead of at API boot or on fork handoff.

Once the sink has opened it cannot be turned off: disabling storage returns 409 with the config unchanged. storageMu keeps the disable check and deferred reconcile aligned with committed (or rolled-back) config so an in-flight start or a failed capture apply cannot leave storage off with the sink open.

The writer now starts with an afterSeq floor from the session so ring events captured while storage was off are not persisted when storage is enabled later. Fork stream resolution no longer binds the parent’s boot S2_STREAM while identity is pending or when the applied payload has no stream.

^{Reviewed by Cursor Bugbot for commit 829b0f7. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Sayan-]

lgtm