fix(chart): stop init-writable-data skipping the chmod when the chown fails (#672)

[shujaatTracebloc]

Fixes#672.

The bug

init-writable-data chained the two repairs:

chown 1000:1000 "$d"&& chmod "$m""$d"||echo"… could not adjust $d …; leaving as-is"

A refused chown short-circuits, so the chmod never runs — and the message says
"leaving as-is", which reads as "nothing could be done" when the one operation that
mattered was never attempted.

Why it's worse than a cosmetic ordering nit

The mode, not the ownership, is what makes these trees usable. kubelet ignores
fsGroup on hostPath (kubernetes/kubernetes#138411), so /data/shared has to be
other-writable for the ingestion Job (uid 65534, or HOST_UID) and the CLI
staging/teardown pod (uid 65532) — neither is 1000, and neither shares a group with it.
The chown to 1000 is cosmetic next to that.

It is also the call most likely to fail: on a Windows/Docker-Desktop bind mount or an
NFS root_squash export, chown is exactly what gets refused while chmod would have
succeeded. So the least important failure was cancelling the most important repair, which
silently nullified the 2777/3777 split from #667on the platform that split was written
for. Losing the chmod is strictly worse than losing the chown. Symptom is #653's
mkdir: can't create directory '/data/shared/.tracebloc-staging/': Permission denied.

The fix

chown and chmod are now separate best-effort statements, each recording whether it
failed, and the per-dir verdict is graded on the mode observed afterwards (ls -ldn)
rather than on either exit status — a bind mount can accept a chmod and ignore it, so an
exit code is not evidence. A partial result is reported as one:

init-writable-data: OK /data/shared other-writable, want 2777 got drwxrwsrwx uid 0 (chown failed; other-writable regardless)
init-writable-data: FAIL /data/shared want 2777 got drwxr-xr-x uid 0 (chown+chmod failed)

The verdict grades other-writability alone — that is what decides whether those uids can
work, and failing a mount that granted other-write while dropping S_ISGID would cry wolf on
a working install. So the label names the one thing it verified and always prints want vs
got, rather than a bare "OK" that could be read as "the whole mode landed".

stderr from the failing call is deliberately not suppressed, so the real errno
(Operation not permitted vs Read-only file system) lands in kubectl logs next to the
verdict. That is the one intentional divergence from Get-ReleaseDirsPrepCommand, which
suppresses it because its output is a user-facing installer line.

Unchanged: per-dir modes (/data/shared 2777 setgid-no-sticky, /data/logs 3777
setgid+sticky), per-dir independence, non-fatal behaviour, POSIX sh for busybox. Kept
diffable by eye against the installer's Get-ReleaseDirsPrepCommand, which already uses
the same path:mode spec and the same ls -ldn verification.

Verified by executing the rendered command, not by reading it

Every run below uses command[2] extracted from helm template output, in busybox:1.35.

Scenario	Result
sh -n, dash -n, bash --posix -n	clean
busybox as root (happy path)	/data/shareddrwxrwsrwx, /data/logsdrwxrwsrwt, uid 1000, exit 0
--cap-drop CHOWN (chown refused, chmod permitted)	modes still landdrwxrwsrwx / drwxrwsrwt, exit 0
…same conditions, old command	both dirs stay drwxr-xr-x while logging "leaving as-is"
/data/shared read-only (both calls fail)	FAIL with the real errno; /data/logs still fixed; exit 0

End-to-end on a shared volume, after a refused chown: uid 65534 creates
.tracebloc-staging/ and writes /data/logs; uid 65532 unlinks uid 65534's entries in
/data/shared (no sticky) but is correctly refused in /data/logs (sticky). Both halves
of the #667 split still hold.

Tests

New #672 case in client/tests/jobs_manager_test.yaml, checked in both directions: it
fails against the old command and passes against the fix. Existing assertions kept and
updated for the multi-line command ((?s) where a pattern now spans lines).

One trap worth flagging for review: the obvious comment-scoped guard
^[^#\n]*chown.*&&.*chmod is silently vacuous here — the loop body legitimately
contains a # (${e#*:}) before the chown, so [^#\n]* can never reach it and the
assertion passes against the very one-liner it exists to reject. I hit this and confirmed
it by running the assertion against the old command. The guard is therefore unscoped, and
the template describes the old shape in words ("chained with a logical AND") rather than in
code so prose can't trip it.

• helm unittest client -f 'tests/jobs_manager_test.yaml': 35 passed (was 34)
• full suite: 380 passed (was 379); develop's baseline of 5 failed / 5 errored / 4 failed
  suites is unchanged — verified against a clean origin/develop checkout, not assumed
• helm lint: unchanged (pre-existing clientId / clientPasswordminLength errors only)
• scripts/chart-version-guard.sh with BASE_SHA=origin/develop: ✓ (1.9.33 → 1.9.34,
  version + appVersion in lockstep)

Second commit: two overclaims in my own reporting

Found by running the failure paths, not reading them — same family as the bug being fixed, so
worth calling out rather than burying:

1. The pass label was a bare OK, which reads as "the whole mode landed" when only
   other-writability was checked. Now OK <dir> other-writable, want … got ….

2. Worse: the partial-result note said "mode applied anyway" whenever a call failed. On a
   dir that was already other-writable where both calls were refused, that is false —
   nothing the container did applied anything. Reproduced in busybox:1.35 (pre-set 1777,
   run as a non-owner uid so both calls are refused):

   want 2777 got drwxrwxrwt uid 0 (chown+chmod failed; mode applied anyway)
   

   Now reads (chown+chmod failed; other-writable regardless) — the observation, not a causal
   claim it cannot support. A notMatchRegex pins the old wording out.

Notes

• The bug is older than the issue says.init-writable-data skips the chmod whenever the chown fails (chown && chmod) #672 attributes the chain to fix(chart): drop the sticky bit on /data/shared so a dataset can be deleted #667. Git says
  otherwise: chown … && chmod … entered in chart 1.9.20 (Truncated tool binary passes #607's size floor and dead-ends at the checksum (make the checksum drive retry) #611/fix(installer): harden the Windows install→first-experiment path — checksum-driven downloads, reliable cluster-create, writable ingest & training volumes, local-chart support (#611) #612, a07f76b) and
  shipped in every version through 1.9.33 — thirteen chart versions, not just since fix(chart): drop the sticky bit on /data/shared so a dataset can be deleted #667.
  fix(chart): drop the sticky bit on /data/shared so a dataset can be deleted #667 (7852f02) rewrote the modes on that exact line and left the control flow alone.
  That widens the blast radius: any hostPath edge installed since 1.9.20 on a mount that
  refuses chown has had a 0755 /data/shared this whole time.

• scripts/install-k8s.ps1 deliberately untouched — fix(installer): pre-create the hostPath PV dirs on Windows so the first ingest works #654 is open against it and already
  does chown/chmod as separate statements. Chart-only change.

• Adds the recurring-finding rule to .cursor/BUGBOT.md, per CLAUDE.md ("a finding that
  recurs across PRs becomes a rule"). This shape has now appeared in fix(installer): pre-create the hostPath PV dirs on Windows so the first ingest works #654 (four findings of
  the same family) and here. The grep-expressible half belongs in code-quality's
  house-rules, which live in tracebloc/.github — out of scope for this PR.

• Fixed a dangling clause in the neighbouring comment block ("…clean up after them; runs as
  root only long enough to fix the mounts") that had lost its subject in an earlier edit.

• Out of scope, spotted while verifying: the dirs the ingest itself creates inside
  /data/shared come out drwxr-sr-x (its own umask), so uid 65532 cannot delete files
  nested inside them — only entries directly in /data/shared. The 2777-no-sticky choice
  fixes the top level only. Not a regression from this PR (on develop the ingest cannot
  create the staging dir at all when the chown is refused), and it depends on the ingestor's
  umask, which lives in another repo. Worth its own ticket.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes privileged init behavior on every hostPath jobs-manager rollout; impact is positive for common mount types but alters logging and permission repair semantics at cluster bootstrap.

Overview
Fixes#672: on hostPath installs, init-writable-data no longer chains chown and chmod with &&, so a refused chown (bind mounts, NFS root_squash, etc.) no longer skips the load-bearing chmod that makes /data/shared and /data/logs usable for non-1000 writers.

The init script is now a multi-line shell block: separate best-effortchown and chmod, per-dir OK/FAIL lines graded on observed mode via ls -ldn (other-write only, with explicit want vs got), always exit 0, and stderr left visible in pod logs. Template comments document the control-flow bug and alignment with the PowerShell installer prep path.

Chart bumps to 1.9.37 (version / appVersion). Tests update existing #611 assertions for the new script shape and add a dedicated #672 case (no chown…&&…chmod, ls -ldn check, no mode applied anyway). .cursor/BUGBOT.md adds a standing rule to flag independent repairs joined by && when the second operation matters.

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

[shujaatTracebloc]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ae7471a. Configure here.}

[shujaatTracebloc]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 4489aba. Configure here.}

[shujaatTracebloc]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 56171e8. Configure here.}

[shujaatTracebloc]

Two corrections to the description above (post-merge)

Leaving the body as-written rather than editing it — the verification lines record what actually ran at the time, and rewriting them would falsify that record. Correcting here instead.

1. The blast radius is wider than stated. The Notes say the chain "shipped in every version through 1.9.33 — thirteen chart versions". That was true when written; develop had moved by merge time. Re-derived just now: the chain entered at a07f76b (chart 1.9.20, #611/#612) and shipped through 1.9.36, so it is seventeen chart versions, and this fix lands in 1.9.37 (not 1.9.34 as the version-guard line records). Any hostPath edge installed on 1.9.20–1.9.36 whose mount refused the chown has had a root:root 0755/data/shared the whole time — that's the range to check against, not 1.9.20–1.9.33.

2. The "Out of scope … Worth its own ticket" note is obsolete — please don't file that ticket. It flags the dirs the ingest creates inside/data/shared coming out drwxr-sr-x and attributes it to the ingest's umask. Both halves have since been superseded:

• The ticket was filed — backend#1833 — and it corrected the diagnosis: the ingestor sets umask(0o002), so the mode was never the problem. The real cause is DEST_PATH inheriting group 1000 from the setgid mount plus a silent S_ISGID strip (the process has no CAP_FSETID), landing it at 0775 group-1000 — where the teardown pod (uid 65532) falls through to other = r-x and cannot unlink anything directly inside. Also noted there: /data/shared/.tracebloc-staging/<table> is written by the CLI stage pod at the same uid as teardown, so it was never at risk — the failure is on the ingestor's FinalDestPrefix tree.
• And it was already fixed before that ticket was filed — closed as a duplicate of data-ingestors#477 (9e20849, DEST_DIR_MODE0o2775 → 0o2777), tracked as Windows: first data ingest fails with Permission denied on /data/shared — installer never pre-creates the hostPath PV dirs (bash does) #653.

So the follow-up this PR asks for is done; acting on the note as written would duplicate a closed ticket using a diagnosis that's been disproven.

Neither correction affects the change that merged — the chart fix itself is unchanged and stands.