# Session Log — 2026-08-25

## Context

User reported that a customer's freshly `install.sh`-installed p4lf systemd
service fails to start, with `systemctl status p4lf` showing the process
looping in `activating (auto-restart)` / `code=exited, status=1/FAILURE`,
preceded by several systemd `Ignoring invalid environment assignment ...`
warnings referencing `/p4/common/bin/p4_vars`.

Note: ai/AGENTS.md at the start of this session described an older set of
requirements (timestamped chunk filenames, MaxLogChunks rotation, README
p4-logtail cleanup) that turned out to already be implemented in the current
code (`internal/chunker/chunker.go` already produces
`log.<YYYY-MM-DD>.<start>.<end>.gz` and enforces `MaxLogChunks`; README already
documents `p4 logtail` is no longer used). The user flagged AGENTS.md as
stale for this session's actual ask (diagnosing the systemd failure) and this
log reflects that diagnostic/fix work instead.

## Root Cause

`p4lf.service` used:

```
EnvironmentFile=-/p4/common/bin/p4_vars
```

`EnvironmentFile=` only understands literal `KEY=VALUE` lines; it silently
ignores (with a WARNING to the journal) any line containing shell syntax
(command substitution `$(...)`, `if`/`export`/conditionals, etc.). SDP's
`p4_vars` is a full bash script, not a simple env file, so essentially none of
its real logic executes under `EnvironmentFile=` — `$P4LOG` / `$LOGS` never
actually get set in the service's environment.

Because the shipped `p4lf.cfg.example` (and therefore a freshly-installed
`p4lf.cfg`, per `install.sh`) ships with `P4LogFile` commented out, and
`config.Load()` requires `P4LogFile` to be set (from config or `$P4LOG`), the
binary exits with `"P4LogFile is required"` on `os.Stderr` and `os.Exit(1)`
before the internal logger is even initialized. Systemd's `Restart=on-failure`
then loops the process every `RestartSec=10s`, matching the reported
`activating (auto-restart)` state. The real "P4LogFile is required" line was
present in the journal but scrolled out of the default 10-line
`systemctl status` view behind the repeated "Ignoring invalid environment
assignment" noise, making it look like a mysterious/silent failure.

This was actually flagged as an unverified risk back in
`session_log_2026-06-25-3.md`: *"SDP environment file: the systemd unit
references /p4/common/bin/p4_vars for the $LOGS and $P4LOG env vars; verify
this path is correct for the target SDP installation."* — it was never
actually verified against a real SDP `p4_vars`, and it doesn't work.

## Fix

Changed `p4lf.service` to source `p4_vars` in a real shell (which can execute
its script logic) instead of via `EnvironmentFile=`, then `exec` the binary so
it inherits PID 1-of-the-cgroup semantics systemd expects for `Type=simple`:

```
Environment=INSTANCE=1
ExecStart=/bin/bash -c 'source /p4/common/bin/p4_vars ${INSTANCE} && exec /p4/common/site/log_feeder/p4lf -config /p4/common/site/log_feeder/p4lf.cfg'
```

`ExecReload=/bin/kill -HUP $MAINPID` still works correctly because `exec`
replaces the bash process image, so `$MAINPID` is p4lf's own PID, not bash's.

Filed job **P4LF-3** (Bug, Severity A) documenting the root cause and fix.
Ran `make check` (vet + fmt + full test suite) — all green; this was a
unit-file-only change, no Go code touched.

## Follow-up / Recommendations Not Yet Done

* Consider making `install.sh` (or a first-run check in `p4lf` itself) fail
  loudly / print a clearer hint when `P4LogFile` isn't set anywhere, so this
  class of misconfiguration is easier to diagnose from `systemctl status`'s
  default truncated view.
* Consider trimming `StandardError=journal` verbosity or adding
  `journalctl -u p4lf -n 50` to install.sh's "next steps" so customers see the
  full failure reason, not just the last 10 lines.
* Have not yet verified against a real customer `/p4/common/bin/p4_vars` file
  (no access) — the fix is based on well-established SDP conventions (sourced
  as `. /p4/common/bin/p4_vars <instance>`). Recommend the user confirm on the
  actual sandbox before wider rollout.

## Follow-up: README install docs, version bump, DeveloperGuide.md

After the systemd fix above was reviewed and submitted (change 33270, job
P4LF-3 closed on submit), did three more things in the same session:

1. **README.md** — added an "Installation" section (right after the intro,
   before "How It Works") summarizing what `install.sh` does and its `-s`,
   `-d`, `-n`, `-h` options, pulled from the script's own header comments.
   This is now the first thing a new reader sees after the intro, per the
   user's request that install instructions be prominent.
2. **Version bump** — `internal/version/version.go`: `SemVer` `1.0.3` →
   `1.0.4`, per explicit user request (no behavior change accompanies this
   bump other than the README/docs work in this session).
3. **DeveloperGuide.md** (new file) — covers required tooling (Go 1.20+,
   make, gofmt, a P4 client), the `Makefile` build/test/release targets, how
   to bump `SemVer` and what Major/Minor/Patch mean for this project, the
   dev → main → release-stream promotion/merge workflow (copy up / merge
   down), and an explanation of why `ai/` and `bin/` are `isolate`d paths in
   the `//p4lf/main` stream spec (dev-support files vs. the shipped
   product).

Ran `make check` after all changes — vet/fmt/tests all green.

## Follow-up: release to main, and customer confirmation

Reviewed and submitted the pending changelists, then walked through a full
release:

1. Submitted change 33270 (p4lf.service fix) and 33271 (README install docs
   + `docs/DeveloperGuide.md` + SemVer 1.0.4), moving `DeveloperGuide.md`
   into `docs/DeveloperGuide.md` per user request (README.md stays at repo
   root since P4 Code Review treats root-level README.md specially), and
   adding a "Developer Guide" pointer link from README.md to it.
2. Built all 4 platform binaries in `dev` (`make build-all`), reconciled
   (`p4 reconcile bin/...`), and submitted as change 33272.
3. Verified `p4 status ...` was clean in `dev`.
4. `p4 switch -l` / `p4 switch main` — switched the same client to
   `//p4lf/main` (illustrating stream switching rather than a second
   workspace).
5. `p4 copy -n -S //p4lf/dev` (dry run) confirmed exactly the expected 4
   files would promote (`README.md`, `docs/DeveloperGuide.md`,
   `internal/version/version.go`, `p4lf.service`) — and confirmed `ai/` and
   `bin/` correctly did **not** appear, since they're isolated paths.
6. `p4 copy -S //p4lf/dev` + submit → change 33273, promoting v1.0.4 to
   `main`.
7. Rebuilt binaries *again*, this time in `main` (`make build-all` →
   `p4 reconcile bin/...` → submit as change 33274) — necessary because
   `bin/` is isolated per-stream and does not come along with the copy-up.
8. Verified `./bin/p4lf-darwin-arm64 -version` reported
   `p4lf 1.0.4 (CL 33273, //p4lf/main#2, ...)` and `p4 status ...` was clean.

**Customer confirmation:** The customer tested the newly released fix on
their sandbox and **confirmed it resolved the systemd startup failure**
(job P4LF-3 issue). No further action needed on that front for now.

### Developer Guide updates (post-release retrospective)

Having just walked through a real dev → main promotion + rebuild end to end,
switched back to `dev` (`p4 switch dev`) and updated
`docs/DeveloperGuide.md`'s "Promoting dev → main" section to reflect actual
practice rather than the earlier, less precise description:

* Documented `p4 switch -l` / `p4 switch <stream>` as the way to move a
  single client between streams (instead of implying a separate `main`
  workspace is required).
* Documented `p4 copy -n -S //p4lf/dev` as a dry-run preview step before the
  real `p4 copy -S //p4lf/dev`.
* Added an explicit, previously-missing step: **rebuild and submit binaries
  in `main` after promoting**, since `bin/` is isolated per-stream and the
  copy-up does not bring binaries along. This was implied by the isolation
  rationale elsewhere in the doc but was not called out as an actionable
  step in the promotion workflow itself — a real gap that would have
  confused a future agent following the doc literally.
* Added verification steps (`-version` check, `p4 status ...`) and a
  reminder to `p4 switch dev` when finished.

### ai/AGENTS.md cleanup

Rewrote `ai/AGENTS.md` to remove stale, task-specific content (the old "NEW
REQUIREMENTS" list for timestamped filenames / MaxLogChunks / README
p4-logtail cleanup — all of which turned out to already be implemented, and
the stale "Current Project State (as of CL 32835)" section). Replaced with
an evergreen structure: Current Status (kept up to date, now reflects v1.0.4
release + customer confirmation + open job P4LF-2), Background Info, Quick
Start, Key Files for Context (now points at `docs/DeveloperGuide.md` too),
Version Control Interaction, and The 'ai/' folder — all general-purpose
content meant to remain useful across many future sessions rather than
describing one task.

Submitted these `ai/` folder updates (AGENTS.md + this session log) as a
dev-only change, per the isolate rule (they don't promote to `main`).

### DeveloperGuide.md wording clarification, then promoted

User pointed out that the "Promoting dev → main" section shouldn't imply
`p4 switch` (single client repurposed across streams) is the only/default
approach — a dedicated workspace per stream is equally valid and, at larger
scale or with more complex workspace mappings, often simpler/more
predictable than `p4 switch`. Updated `docs/DeveloperGuide.md` to present
both as valid options, note `p4 switch`'s caveats, and stop implying it's
required. Submitted as change 33276 (dev), then promoted doc-only to `main`
as change 33277 (no binary rebuild needed — the copy-up preview correctly
showed only the doc file changing, no `bin/` deltas).

**Observed gap (noted, not fixed):** promoting a doc-only change to `main`
has no equivalent of the binary's `-version` stamp to say "what doc content
is in main as of when." Binaries get a SemVer + CL stamp baked in via
`internal/version/version.go`; docs (README.md, docs/DeveloperGuide.md) do
not carry any analogous version marker — you'd have to check `p4 changes`/
`p4 filelog` on the specific file to know what's current. This is fine for a
project this size and won't be addressed now, but is worth keeping in mind
if it ever causes confusion (e.g., "which version of the docs is live in
main?").

## Follow-up: p4lf.cfg root-ownership bug (P4LF-4)

A customer reported a *different* failure on a fresh `install.sh` install:
`p4lf.service` kept auto-restarting (exit-code 1). Their own workaround was
`sudo chown perforce:perforce /p4/common/site/log_feeder/p4lf.cfg`, which
fixed it — a strong hint at root cause.

Confirmed by inspection: `p4lf.service` runs the binary as
`User=perforce`/`Group=perforce`, but `install.sh` (which must run as root)
installed `p4lf.cfg` and `p4lf.cfg.example` via plain
`install -m 0640 ...` with no `-o`/`-g`, so both ended up root-owned. The
perforce user then had no read access to its own config — exactly matching
the customer's exit-code-1 auto-restart loop and their working fix.

Filed **P4LF-4** (Bug, severity A) and fixed in `install.sh`:

* Added `PERFORCE_USER`/`PERFORCE_GROUP` constants (`perforce`/`perforce`,
  matching `p4lf.service`'s `User=`/`Group=`).
* Before installing config files, check `id -u "$PERFORCE_USER"`; if found,
  pass `-o perforce -g perforce` to `install` for both `p4lf.cfg` and
  `p4lf.cfg.example`. If not found (e.g. a non-SDP host, or local
  testing/dry-run environments without that OS user), fall back to
  root-owned files but print an explicit warning naming the exact `chown`
  command needed — never silently ship a broken config.
* On upgrades/re-installs where `p4lf.cfg` already exists (and is thus
  preserved, not overwritten), added a `chown` call to re-assert correct
  ownership — so hosts previously bitten by this bug get healed by simply
  re-running `install.sh`, without requiring a manual chown.
* Updated dry-run output to reflect the new `install -o/-g` invocations and
  the ownership-reassertion chown.

Verified: `bash -n install.sh` (syntax), and extracted/ran just the
`CFG_OWNER_ARGS` array-building + `install` logic in isolation (since a full
run requires root and network access to workshop.perforce.com) — confirmed
the empty-array fallback path works cleanly with `set -u` and produces a
correctly-permissioned file.

Submitted as change 33291 in `dev` (fixes P4LF-4), then promoted to `main`
as change 33292 (script-only change — `p4 copy -n -S //p4lf/dev` confirmed
only `install.sh` differed; no `bin/` rebuild needed). Not yet confirmed by
the customer as of this writing — `ai/AGENTS.md` Current Status updated
accordingly.