Skip to content

fix: give CEF a per-instance cache directory on native runs - #671

Open
wagenet wants to merge 2 commits into
Eyevinn:mainfrom
wagenet:fix/cef-cache-location
Open

fix: give CEF a per-instance cache directory on native runs#671
wagenet wants to merge 2 commits into
Eyevinn:mainfrom
wagenet:fix/cef-cache-location

Conversation

@wagenet

@wagenet wagenet commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Description

Only one Strom instance per machine can use cefsrc (HTML sources, DSK graphics) when running natively. Starting a cefsrc flow on a second instance fails:

GstCefSrc:c: gst_base_src_start (): Failed to start
→ HTTP 500 "Pipeline state change failed: Element failed to change its state"

Chromium keys its process singleton on the cache/profile directory. strom-full sets GST_CEF_CACHE_LOCATION=/tmp/cef-cache in its entrypoint, so containers are unaffected — but native runs never set it and inherit Chromium's default, which CEF itself warns about on every startup:

WARNING:cef/libcef/common/resource_util.cc:83] Please customize
CefSettings.root_cache_path for your application. Use of the default value
may lead to unintended process singleton behavior.

This defaults the variable to a directory derived from the data directory, so instances with different data directories are isolated. An explicitly configured GST_CEF_CACHE_LOCATION still wins, and Docker is unaffected since the entrypoint sets it before we get here.

Testing

macOS arm64, one headless instance starting a cefsrc flow:

cache flow start
default (in-memory), before this change 7.1s
per-instance, cold 22.9s (one-off, builds an 11MB profile)
per-instance, warm 0.9s

Concurrent instances, distinct data directories:

instance A instance B
forced to share one cache dir 200 500 — fails
per-instance (this change) 200 200

So the cost is a one-time penalty the first time a data directory is used, and repeat starts get substantially faster.

Trade-off worth a maintainer's opinion

This moves native runs from an in-memory cache to an on-disk one, which means Chromium profile state (cookies, local storage, HTTP cache) now persists across restarts where it previously did not. For broadcast graphics that is usually desirable — and it matches what the Docker image already does — but it is a behaviour change, and I would rather you decide than have me assume. Two alternatives if you'd prefer:

  1. Put the cache under the OS temp/cache dir instead of the data directory, keyed by port or PID, so nothing persists.
  2. Keep it opt-in behind a config flag and only fix the warning.

Happy to switch to either.

Related Issue

None.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have run cargo fmt --all — clean
  • I have run cargo clippy --workspace --all-targets -- -D warnings — fails on main independently of this change: 188 pre-existing float-literal-f32-fallback errors in frontend/*.rs on Rust 1.97.1 (189 error lines incl. the aggregate). Zero errors in the file touched here. Addressed by fix(frontend): pin ambiguous float literals to f32 for new rustc lint #664.
  • I have run cargo test --workspace — passes
  • I have updated documentation as needed — no doc change; env var is already documented behaviour in the Docker entrypoint
  • I have added tests for new functionality — this is process-global env setup before the runtime starts; I did not find a non-tautological way to test it in the existing harness. Suggestions welcome.

🤖 Generated with Claude Code

Only one Strom instance per machine can use `cefsrc` (HTML sources, DSK
graphics) when running natively. The second instance fails to start any
cefsrc element:

    GstCefSrc:c: gst_base_src_start (): Failed to start
    -> HTTP 500 "Element failed to change its state"

Chromium keys its process singleton on the cache/profile directory. The
strom-full Docker image sets GST_CEF_CACHE_LOCATION=/tmp/cef-cache in its
entrypoint, so containers are fine, but native runs never set it and get
Chromium's default -- which CEF itself warns about at startup:

    Please customize CefSettings.root_cache_path for your application. Use
    of the default value may lead to unintended process singleton behavior.

Default the variable to a directory derived from the data directory, so
instances with different data directories are isolated. An explicitly
configured GST_CEF_CACHE_LOCATION still wins.

Measured on macOS arm64, one headless instance starting a cefsrc flow:

    default (in-memory)     7.1s
    per-instance, cold     22.9s   (one-off, builds an 11MB profile)
    per-instance, warm      0.9s

So the cost is a one-time first-run penalty on a fresh data directory, and
repeat starts get faster. Two concurrent instances with distinct data
directories both start successfully; forced to share one cache directory,
the second still fails as before.
@wagenet
wagenet marked this pull request as ready for review August 11, 2026 03:58
srperens
srperens previously approved these changes Aug 21, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Intent: Set GST_CEF_CACHE_LOCATION to a per-instance directory derived from the configured data path, so multiple Strom instances on the same machine each get an isolated Chromium profile and avoid the process-singleton conflict that blocks cefsrc from starting.

Risk: Very low. The env var is only set if it is not already set (explicit config always wins), the change is guarded by if let Some(data_dir), and it happens before GStreamer initialisation on a single-threaded stack (no set_var concurrency hazard).


What looks good:

  • The is_none() guard correctly preserves GST_CEF_CACHE_LOCATION when the operator has set it explicitly (e.g. the strom-full Docker entrypoint).
  • Deriving from flows_path.parent() is a natural choice — CEF state should live next to flow data, giving it a warm on-disk profile that also cuts subsequent start latency.
  • The if let Some(data_dir) guard handles the (unlikely) case where flows_path has no parent.

One suggestion (non-blocking):

If flows_path.parent() returns None, nothing happens and CEF falls back to its default (shared) cache. This silent fallback could still trigger the process-singleton warning on unusual configurations. A debug! or warn! log line in the else branch would make that case visible:

} else {
    debug!("CEF cache isolation skipped: flows_path has no parent directory");
}

Otherwise the change is clean and well-motivated.

srperens
srperens previously approved these changes Aug 21, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewing PR #671: fix: give CEF a per-instance cache directory on native runs
Verified against 7d43002 vs main 317e80b.

This supersedes the earlier unverified approval on this PR, which lacked code citations. Verdict unchanged: approve.

Summary

  • Sets GST_CEF_CACHE_LOCATION to a per-instance path derived from config.flows_path at startup, so each native Strom instance gets an isolated Chromium profile.
  • The env var is only set when not already configured, preserving Docker container behaviour where the entrypoint pre-sets it.

Claim verification

Claim Verdict Evidence
Change is gated — explicit env config always wins CONFIRMED backend/src/main.rs:316 if std::env::var_os("GST_CEF_CACHE_LOCATION").is_none()
if let Some(data_dir) handles flows_path with no parent CONFIRMED backend/src/main.rs:318 if let Some(data_dir) = config.flows_path.parent()
Set before GStreamer initialisation (single-threaded, no set_var hazard) CONFIRMED backend/src/main.rs ordering: CEF env var set at line 317 → init_logging at line 328 → pipeline setup. No threads spawned before this point.
Docker container unaffected CONFIRMED backend/src/main.rs:311 code comment: "The strom-full Docker image sets GST_CEF_CACHE_LOCATION in its entrypoint" — env var already present, is_none() guard skips.
Chromium process singleton bug is the underlying cause UNVERIFIED (external) CEF/Chromium singleton behaviour; not reproducible from this repo's code. The Chromium warning log cited in the PR description is well-established upstream CEF behaviour.

Test coverage — no test claimed. A test of CEF singleton isolation requires running two Chromium instances, which is not feasible in CI. Acceptable for this change.

Risks / edge cases

  • If flows_path.parent() is None (e.g. flows_path = "/flows.json"), the env var is silently skipped and CEF falls back to the shared default — the multi-instance bug would persist with no log line. Low likelihood in practice; a debug! in the else branch would make the silent skip visible.
  • std::env::set_var is deprecated as unsafe in Rust ≥1.81 due to unsoundness in multi-threaded contexts. This call is on a genuinely single-threaded startup path so there is no functional hazard, but worth tracking as the Rust stdlib evolves.

Requested changes — none.

Confidence: HIGH

@srperens
srperens dismissed their stale review August 21, 2026 11:03

Superseded by v2-protocol review; original was unverified (no evidence gate).

@srperens
srperens dismissed their stale review August 21, 2026 11:23

Retracting this approval: it was produced before the review protocol required CI verification, and no CI had run on this PR at the time (the workflow run was stuck awaiting maintainer approval, so zero checks existed). The run has now been approved; this PR will be re-reviewed once checks report.

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewing PR #671

head=7d430024 (7d43002) vs main=317e80b1 (317e80b)

Summary

Gives the cefsrc (CEF/Chromium HTML source) element a per-instance GST_CEF_CACHE_LOCATION on native (non-Docker) runs, derived from the configured data directory, only when the env var isn't already explicitly set. Fixes a second-instance-on-the-same-machine startup failure caused by Chromium's process-singleton behavior when multiple instances share the default cache location.

Claim verification

# Claim Verdict Evidence
1 GST_CEF_CACHE_LOCATION is only set when not already present in the environment (explicit config always wins) CONFIRMED CODE: backend/src/main.rs diff — if std::env::var_os("GST_CEF_CACHE_LOCATION").is_none() { ... }
2 Directory is derived from the data directory (config.flows_path.parent()) CONFIRMED CODE: std::env::set_var("GST_CEF_CACHE_LOCATION", data_dir.join("cef-cache")); inside the if let Some(data_dir) = config.flows_path.parent() guard
3 Docker (strom-full) is unaffected since its entrypoint already sets the var CONFIRMED (by absence of new conflict) CODE: the is_none() guard means an already-set var (as the Docker entrypoint does) is left untouched; no code in this diff runs in a way that could override it
4 std::env::set_var usage is consistent with existing codebase patterns (not introducing a new unsafe/unprecedented call) CONFIRMED CODE: backend/src/main.rs:282 (pre-existing, unmodified) already calls std::env::set_var("WAYLAND_DISPLAY", ""); backend/src/config.rs has several more (lines 500, 513, 535, 550, 609, 619) — same pattern, edition 2021 (Cargo.toml:8), compiles without unsafe

Diagnosis

Small, well-scoped, single-file change. The guard against overriding an explicitly-set value is correct and matches the stated Docker-compatibility goal. One edge case not explicitly handled: if config.flows_path is a bare relative filename with no directory component (the documented default fallback is PathBuf::from("flows.json"), per backend/src/config.rs:380), Path::parent() returns Some("") rather than None — the if let Some(data_dir) branch still executes, and "".join("cef-cache") resolves to a relative cef-cache directory under the process's current working directory rather than a clearly-scoped data directory. This isn't a crash and still achieves per-instance isolation as long as each instance's CWD differs, but the "isolated per data directory" framing weakens silently in this fallback case. Worth a maintainer note, not a blocker.

Blast radius

LOCAL — one env-var side effect at startup in backend/src/main.rs, gated behind an is_none() check, only affects cefsrc-using flows (HTML sources, DSK graphics) on native (non-Docker) runs. No API/type/WebSocket contract changes.

Tests & CI

The PR's own checklist candidly discloses no new automated test was added ("process-global env setup before the runtime starts; I did not find a non-tautological way to test it") — reasonable given the nature of the change (process-startup env var side effect).

CI: gh api .../commits/7d430024.../check-runs → 2 jobs failed: Check (Linux) and Check & Build (WASM), both due to error: falling back to \f32` as the trait bound `f32: From` is not satisfiedinfrontend/src/themes.rs(187 total errors in that file/crate). This diff touches onlybackend/src/main.rsgit diff origin/main...pr671 --stat -- frontend/returns empty, confirming zero overlap with the failing file. This run (id31456538888) was created_at: 2026-08-10but only actually executedrun_started_at: 2026-08-21T11:22— an 11-day gap consistent with the same GitHub Actions fork-PRaction_requiredapproval-gate delay found on otherwagenet/*PRs in this batch. By the time it finally ran today, the floatingdtolnay/rust-toolchain@stablehad drifted to a newer Rust version that regressed this float-inference diagnostic;main's own last recorded CI run (317e80b`, 2026-08-19) was green, before this drift. This is a pre-existing, repo-wide toolchain-drift breakage, not something introduced by this diff — but per this review protocol, red CI on the diff mechanically blocks approval regardless of root-cause attribution.

Risks / edge cases

  • Native runs now persist Chromium profile state (cookies, local storage, HTTP cache) across restarts, where previously (in-memory default) they did not — the PR author explicitly flags this as a behavior change and asks for a maintainer decision rather than assuming; this is honestly disclosed, not hidden.
  • Relative-flows_path-fallback edge case noted above (minor).

Requested changes

None blocking on correctness. Recommend a maintainer explicitly weigh in on the persistence-behavior trade-off the author flagged (on-disk profile persistence vs. two suggested alternatives), and recommend the repo pin the Rust toolchain version (see run summary) to stop this class of CI failure from recurring on every long-pending fork PR.

Confidence: HIGH — single-file, narrowly-scoped, all claims directly verified against code; CI failure is conclusively root-caused to an unrelated pre-existing toolchain-drift issue, not this diff. Verdict: Comment (mechanical CI gate), not Approve.

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewing PR #671: fix: give CEF a per-instance cache directory on native runs
Verified against 0d9f28a vs main 57a0040.

This supersedes my earlier review on this PR (a protocol=v2 review, id 4992798907, COMMENTED, head 0d9f28a6 — same head as this one, no new commits since). That review predates the CI-verification and evidence-citation requirements of this protocol version and should not be treated as an independent second opinion. It is a plain COMMENTED review, which GitHub does not allow dismissing via API — this note is the supersession record instead.

Summary

  • On native (non-Docker) runs, if GST_CEF_CACHE_LOCATION isn't already set, derives it from the resolved data directory (<data-dir>/cef-cache) instead of leaving Chromium on its in-memory default.
  • Fixes a real failure: two native instances sharing Chromium's default profile/cache directory collide on its process-singleton lock, so the second instance's cefsrc fails to start.
  • Explicitly flags a trade-off for the maintainer to decide: this moves native runs from an in-memory CEF cache to a persistent on-disk one (cookies/local-storage/HTTP-cache survive restarts).

Claim verification

Claim Verdict Evidence
Docker sets GST_CEF_CACHE_LOCATION in its entrypoint, so containers are unaffected by this change CONFIRMED docker/strom-full/entrypoint.sh:74 export GST_CEF_CACHE_LOCATION="/tmp/cef-cache"; the new code only acts if std::env::var_os("GST_CEF_CACHE_LOCATION").is_none() (backend/src/main.rs:316), so an already-Docker-set value always wins.
An explicitly configured GST_CEF_CACHE_LOCATION always wins CONFIRMED Same is_none() guard, backend/src/main.rs:316.
Chromium keys its process singleton on the cache/profile directory, causing the reported 500 on a second instance EXTERNAL This is Chromium/CEF's internal behavior, not code in this repo — I did not inspect CEF/Chromium source to confirm; taking the PR's account (backed by the quoted upstream warning about CefSettings.root_cache_path) at face value.
The new cache directory is derived from the data directory so "instances with different data directories are isolated" CONFIRMED, but see Diagnosis for scope backend/src/main.rs:317-318 if let Some(data_dir) = config.flows_path.parent() { std::env::set_var("GST_CEF_CACHE_LOCATION", data_dir.join("cef-cache")); }.
This fixes "only one Strom instance per machine can use cefsrc" as a general statement CONTRADICTED for the zero-config case backend/src/paths.rs:100-121 fn default_data_dir(): outside Docker it returns ProjectDirs::from("com", "eyevinn", "strom").data_dir() — a single fixed platform path with no port/PID/instance component. Two native instances started with no --data-dir/STROM_DATA_DIR override still resolve to the same flows_path, hence the same cef-cache directory, hence the same collision this PR is meant to fix. The PR's own verification table is titled "Concurrent instances, distinct data directories" — it did not test the zero-config case.

Diagnosis
Right mechanism, right layer (env var set once at startup before GStreamer/CEF initializes, matching how the Docker entrypoint already does it) — this is not symptom suppression. But it is BOUNDED, and the bound is narrower than the PR title suggests: it only isolates instances that already have distinct data directories, which requires the operator to pass --data-dir/STROM_DATA_DIR (or an equivalent distinct --flows-path) per instance. docs/POSTGRESQL.md:9 ("Each Strom instance should have its own database") establishes that Strom already expects operators to give concurrent instances distinct storage, so this is plausibly consistent with the documented operational model — but that expectation isn't stated anywhere for the JSON-storage/native-run case this PR touches, and the "second instance" scenario in the bug report reads like a default/naive run. Worth confirming with whoever hit this originally whether they were already using distinct data directories; if not, this PR doesn't fix their case as filed.

Blast radius
LOCAL — a single if block in fn main() before logging/GStreamer init, gated to run only when GST_CEF_CACHE_LOCATION is unset. grep -rn "GST_CEF_CACHE_LOCATION" backend docker shows exactly two producers (this new code and the Docker entrypoint) and no other reader in this repo (the consumer is the external gstcefsrc element). Behavior change is real but narrow: native users who never set GST_CEF_CACHE_LOCATION get a persistent on-disk CEF profile where they previously got Chromium's in-memory default — this is the trade-off the PR explicitly asks the maintainer to weigh in on, and I'm not aware of anything in this repo that already answers it (no existing policy on CEF profile persistence that I could find).

Tests & CI

  • Check (Linux), Build (Linux ARM64), Build (Linux x86_64), API Contract Check, Check & Build (WASM) — all pass. Build (Windows)/Build (macOS) — skipping; not required here since the diff has no #[cfg(target_os = ...)] gates and uses only cross-platform std::env/std::path calls, so there's no platform-specific compilation risk this diff introduces.
  • No automated test was added. The PR's checklist is upfront about this: "this is process-global env setup before the runtime starts; I did not find a non-tautological way to test it in the existing harness." That's accurate for testing std::env::set_var itself (process-global, not thread-safe to exercise in a parallel test suite), but the decision logic — "does the derived path vary with flows_path's parent, and does an existing value win" — doesn't need to touch a real env var to be tested. The smallest fix (below) extracts that into a pure function.
  • The perf numbers in the PR body (7.1s / 22.9s / 0.9s cold-vs-warm; the two-instance 200/500-vs-200/200 table) are manual, local, macOS-only measurements; nothing in CI exercises or reproduces them, so they're UNVERIFIED from this repo's evidence — not because macOS CI is skipped (this diff isn't macOS-specific), but because no automated test captures this behavior at all, on any platform.
  • No relevant canary: no closures/signal handlers touched, so pipeline_lifecycle_test.rs isn't implicated; no endpoint/type change, so openapi_test isn't implicated.

Risks / edge cases

  • The zero-config concurrent-instance case (both cited above) — the PR fixes the "distinct data directories" case, not the case the title's "only one Strom instance per machine" framing implies.
  • SPECULATIVE (not verified): if an operator explicitly sets STROM_FLOWS_PATH to a bare relative filename with no directory component (bypassing --data-dir entirely), Path::parent() could return an empty-but-Some path, sending the cache under the process's current working directory rather than a stable per-instance location — I did not trace whether any existing code path produces that shape of flows_path in practice (the documented defaults in paths.rs:56-61 always join onto an absolute or ./data-relative base_dir, so this would require a deliberately unusual override).

Requested changes

  1. Extract the if var_os(..).is_none() { ... } block (backend/src/main.rs:316-320) into a small pure function, e.g. fn resolve_cef_cache_location(existing: Option<&OsStr>, flows_path: &Path) -> Option<PathBuf>, and add a unit test asserting: (a) an existing value is returned unchanged, (b) two different flows_path parents produce two different results. This directly addresses the checklist's self-reported test gap without needing to mutate real process env state in the test.
  2. Not a required code change, but the maintainer's opinion is the actual blocker per the PR body's own ask: confirm whether the on-disk-persistent-cache trade-off is acceptable, or whether one of the two alternatives (OS temp dir keyed by port/PID, or opt-in behind a flag) is preferred.

Confidence: HIGH

@srperens

Copy link
Copy Markdown
Collaborator

Thanks for this — and sorry for the slow reply, you asked for a maintainer opinion and then waited ten days for one.

Answering your two open questions first, then the one change I'd like before merge.

Persistence: keep it on disk

Take the warm cache. 7.1s → 0.9s on repeat flow starts is worth more than the in-memory semantics we had, and for broadcast graphics a warm HTTP cache and surviving local storage are usually what you want. Anyone who disagrees can delete the directory or point GST_CEF_CACHE_LOCATION at a tmpfs.

One correction to the reasoning, though: this isn't parity with the Docker image. docker/strom-full/entrypoint.sh sets the location but rm -rfs it immediately before mkdir -p, so containers get a cold profile on every start. Persisting across restarts is genuinely new behaviour rather than existing behaviour extended to native runs — worth phrasing it that way in the PR body, since that's the part a reviewer has to consciously accept.

The mechanism: right idea, and I'd like the derivation changed

Keying on the instance's data directory is correct, and I don't think a better mechanism exists — root_cache_path is Chromium's process singleton key, so a distinct path per instance is the only lever there is. (Keying on port instead would lose the warm cache on every port change and buy nothing, since two instances sharing a data directory are already broken — both write flows.json. A PID-keyed temp dir throws away the 0.9s, which is the best part of this PR.)

What I'd like changed is config.flows_path.parent(). flows_path is independently overridable — --flows-path, STROM_FLOWS_PATH, storage.flows_path — so its parent is only incidentally the data directory:

  • A bare relative override is the sharp edge: Path::new("flows.json").parent() is Some(""), not None, so the if let Some(...) doesn't guard it and join("cef-cache") yields cef-cache relative to the CWD. Two instances launched from the same working directory with different --data-dir then collide again — the exact failure this PR fixes.
  • Two instances with distinct data directories but flows_path pointed at one shared config directory collide too, and an 11MB Chromium profile gets written into a directory the operator thinks of as configuration.
  • In Postgres mode flows_path is resolved but unused, so the cache lands beside a file that doesn't exist. Harmless, but confusing.

The good news is that DataPaths::resolve() already computes exactly the value you want as base_dir, and then drops it on the floor. So:

  1. Add data_dir: PathBuf to DataPaths and Config, carrying base_dir through instead of discarding it, and derive data_dir.join("cef-cache") from that. No Option, no parent(), nothing to guess.
  2. While you're in there, make it a first-class path alongside media_path: storage.cef_cache_path in .strom.toml. GST_CEF_CACHE_LOCATION then stays what it should be — the last-resort override for GStreamer-native config — and the precedence becomes explicit rather than implied.

Test

That change also answers your checklist note. Once the path comes from DataPaths it's no longer process-global env setup before the runtime starts, and there's a non-tautological test available in paths.rs: two PathConfigs with distinct data_dir resolve to distinct cache directories, and an explicit flows_path override does not move the cache. Both fail if the derivation regresses to parent(). That's a guard rather than a demonstration.

Minor

The env var is currently set before init_logging, so the chosen path can't be logged. Move it after and info! it — operators will want to know where the profile went, given the one-off 11MB and the 22.9s cold start.

Also: if you can re-run the concurrent-instances table against the data_dir version, that would be appreciated. You have the two-instance macOS setup and we don't, so that measurement is yours to make.

No concerns on the persistence question — it's the derivation and the test I'd like moved before merge.

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewing PR #671: fix: give CEF a per-instance cache directory on native runs
Verified against 0d9f28a vs main 57a0040.

This updates my earlier v3 review at the same head (posted 2026-08-21T12:13:43Z) — a substantial maintainer comment (2026-08-21T12:19:14Z) arrived after that review with specific technical findings and a change request. The code is unchanged since then; this review independently verifies that comment's claims and reaches its own verdict.

Summary

  • Sets GST_CEF_CACHE_LOCATION to config.flows_path.parent()/cef-cache at startup when the env var isn't already set, so each native instance gets an isolated CEF/Chromium profile directory instead of sharing the default (which serializes on Chromium's process singleton).
  • Framed as parity with the Docker image's strom-full entrypoint.

Claim verification

Claim Verdict Evidence
CEF cache dir derives from the data directory via config.flows_path.parent() CONFIRMED backend/src/main.rs:317 if let Some(data_dir) = config.flows_path.parent() {
Env var is skipped if already explicitly set (Docker/operator override wins) CONFIRMED backend/src/main.rs:316 if std::env::var_os("GST_CEF_CACHE_LOCATION").is_none() {
"it matches what the Docker image already does" (persistence) CONTRADICTED docker/strom-full/entrypoint.sh:74-76: export GST_CEF_CACHE_LOCATION="/tmp/cef-cache" immediately followed by rm -rf /tmp/cef-cache then mkdir -p /tmp/cef-cache — Docker gets a cold profile on every container start, not a persisted one. This PR gives native runs new persistence behaviour, not existing behaviour extended.
"I did not find a non-tautological way to test it" CONTRADICTED backend/src/paths.rs:191-234 (test_resolve_with_data_dir, test_individual_paths_override_data_dir) shows the existing, CI-executable pattern for exactly this kind of assertion (distinct PathConfig → distinct resolved paths) already exists in this file for DataPaths::resolve. It doesn't cover this PR's logic today only because the derivation lives inline in main() rather than in DataPaths — moving it (see Requested changes) makes the same pattern apply directly.
flows_path is independently overridable (--flows-path, STROM_FLOWS_PATH, storage.flows_path), so its parent is only incidentally the data directory CONFIRMED backend/src/main.rs:151-152 #[arg(long, env = "STROM_FLOWS_PATH")] flows_path: Option<PathBuf>; backend/src/config.rs:78 flows_path: Option<PathBuf> on StorageConfig, merged at backend/src/config.rs:256-257 figment.merge(Serialized::default("storage.flows_path", fp))
DataPaths::resolve() computes base_dir and drops it rather than exposing it CONFIRMED backend/src/paths.rs:43-47 computes base_dir; the returned struct backend/src/paths.rs:12-19 (DataPaths { flows_path, blocks_path, media_path }) has no base_dir/data_dir field
In Postgres mode, flows_path is still resolved even though the JSON file isn't used CONFIRMED backend/src/config.rs:279 let data_paths = DataPaths::resolve(path_config)?; runs unconditionally, not gated on database_url; backend/src/config.rs:283 assigns flows_path: data_paths.flows_path regardless
A bare relative --flows-path/STROM_FLOWS_PATH (e.g. flows.json) makes .parent() return Some("") rather than None, so the guard at main.rs:317 doesn't exclude it and the cache lands at a CWD-relative cef-cache EXTERNAL This is std::path::Path::parent() semantics, not repo code — I could not run rustc in this sandbox to confirm empirically (which rustc → not found). Per documented std behaviour, parent() returns Some("") for a single-component relative path and only None for root/empty paths. Assuming that documented behaviour holds, the code at backend/src/main.rs:317 does not special-case an empty parent, so this trigger is real.

Diagnosis

  • Right general idea (give CEF an isolated, instance-scoped cache dir) and the right root cause (Chromium's process singleton keyed on cache/profile dir — EXTERNAL, per the CEF warning text quoted in the PR body). Wrong specific derivation: keying off config.flows_path.parent() instead of the already-computed base_dir/data_dir reintroduces the exact failure this PR exists to fix, through two triggers this diff doesn't close:
    1. Bare relative --flows-path/STROM_FLOWS_PATH (EXTERNAL row above) — two instances launched from the same CWD with different --data-dir collide again on a CWD-relative cef-cache.
    2. --flows-path/STROM_FLOWS_PATH pointed at a directory shared across instances but distinct from each instance's --data-dir — collides again, and this one needs no relative-path quirk, it's a direct consequence of flows_path being independently overridable (CONFIRMED row above).
  • BOUNDED: works correctly only when flows_path is left at its default (i.e., derived purely from --data-dir/STROM_DATA_DIR). That's very likely the common case, but it's an implicit, unstated assumption the code doesn't enforce.
  • Not a request/retry mechanism, so the cost-of-response analysis doesn't apply here.

Blast radius

  • LOCAL. GST_CEF_CACHE_LOCATION is set once at startup in backend/src/main.rs before any pipeline construction; grep -rn "GST_CEF_CACHE_LOCATION" backend/ returns only the three lines cited above — no other call site in this repo reads or writes it, so there's no cross-module fan-out to check. No thread, timer, probe, or pipeline object is created by this code — it's a one-shot std::env::set_var before init_logging, so there's no teardown path to trace.

Tests & CI

  • Checks on 0d9f28a6: Build (Linux x86_64) success, Build (Linux ARM64) success, Check (Linux) success, Check & Build (WASM) success, API Contract Check success, sccache preflight success, Build (macOS) skipped, Build (Windows) skipped (all completed ~12:0x–12:13 UTC 2026-08-21). No platform-specific cfg gates are touched by this diff (plain main() logic, not #[cfg(target_os = "macos")]/#[cfg(windows)]), so the macOS/Windows skip is not a coverage gap for this particular change.
  • No test exists for the changed logic at all — confirmed by grep -rn "GST_CEF_CACHE_LOCATION" backend/ returning only the three main.rs lines, none in a #[cfg(test)] module. Per CLAUDE.md's Tests section ("A regression test must exercise the code it guards"), and given the Claim-verification row above showing a non-tautological test is achievable once the derivation moves into DataPaths, the PR's "not feasible" checklist note doesn't hold up as-is.
  • No canary applies: this diff touches neither pipeline/element construction (pipeline_lifecycle_test.rs n/a) nor API types/endpoints (openapi snapshot test n/a).

Risks / edge cases

  • Relative-flows_path CWD collision — EXTERNAL assumption stated above (Path::parent() semantics), code-side guard at backend/src/main.rs:317 confirmed not to special-case it.
  • Shared-but-distinct flows_path/data_dir collision — CONFIRMED via backend/src/config.rs:256-257 and backend/src/main.rs:317 cited above; this is the same class of bug the PR fixes, just a different config combination.
  • Postgres-mode cache placed beside a flows.json that's never read — CONFIRMED via backend/src/config.rs:279 cited above; cosmetically confusing, not a functional break (no code reads that file in Postgres mode by the same construction).
  • GST_CEF_CACHE_LOCATION is set before init_logging (backend/src/main.rs:316 runs before the init_logging call at backend/src/main.rs:323), so the chosen path is never logged — an operational/debuggability gap, not a correctness one.

Requested changes

  1. Add a data_dir: PathBuf field to DataPaths (backend/src/paths.rs:12-19) and populate it from the already-computed base_dir (backend/src/paths.rs:43-47) in the Ok(Self { .. }) return (backend/src/paths.rs:93-97); thread it onto Config (backend/src/config.rs:120-153, backend/src/config.rs:281-291). In backend/src/main.rs:317, derive the cache path from config.data_dir.join("cef-cache") instead of config.flows_path.parent(). This removes both open collision triggers listed in Diagnosis, since data_dir isn't independently overridable the way flows_path is.
  2. Add a unit test in backend/src/paths.rs, following the existing test_resolve_with_data_dir / test_individual_paths_override_data_dir pattern (backend/src/paths.rs:205-234): two PathConfigs with distinct data_dir resolve to distinct data_dir fields, and an explicit flows_path override does not change it. This is a real regression guard — it fails if the derivation reverts to flows_path.parent().
  3. Minor: move the GST_CEF_CACHE_LOCATION block (backend/src/main.rs:316-320) to after the init_logging call (backend/src/main.rs:323-327) and add an info! logging the chosen path, so operators can see where the profile went.

Confidence: HIGH

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.

2 participants