fix: harden log directory management for ARC/DinD and rootless Docker#5963
Conversation
Address findings from the log directory audit (#5962): - Switch squid-logs from onCreate to onAfterEnsure so ownership is repaired even when the directory pre-exists from a timeout-killed run. Previously only newly-created dirs got chown(13:13); now pre-existing dirs with wrong ownership are also fixed. The container entrypoint preflight remains as defense-in-depth. - Add tolerance for chmod failure on squid-logs (double fallback: if chown fails AND chmod fails, continue silently — the container entrypoint Layer 2 will handle it). - Add age-based pruning for /tmp/gh-aw/mcp-logs: remove subdirectories older than 24 hours on each AWF invocation to prevent unbounded growth on persistent runners. - Document the triple-layer defense pattern for squid log permissions directly in the code (workdir-setup.ts) explaining what each layer does and why none should be removed. - Add log directory layout documentation to docs/logging_quickref.md covering the proxyLogsDir path asymmetry, ownership model, and ARC/DinD triple-layer defense. - Add tests: pre-existing squid-logs ownership repair, double-fallback tolerance, MCP log directory pruning (stale removal, file skipping, error tolerance). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.82% | 98.82% | ➡️ +0.00% |
| Statements | 98.75% | 98.75% | ➡️ +0.00% |
| Functions | 99.72% | 99.72% | ➡️ +0.00% |
| Branches | 95.17% | 95.15% | 📉 -0.02% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
97.6% → 97.8% (+0.25%) | 97.6% → 97.8% (+0.26%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
This PR hardens AWF log directory management for ARC/DinD and rootless Docker by ensuring squid log ownership repairs run even when directories pre-exist, and by preventing unbounded growth of host-level MCP gateway logs. It also documents the log directory layout and the layered permission model to reduce regression risk.
Changes:
- Switch squid logs directory repair from
onCreatetoonAfterEnsurewith best-effortchown→chmodfallback. - Add best-effort pruning of stale subdirectories under
/tmp/gh-aw/mcp-logs(older than 24 hours). - Document log directory layout and the “triple-layer defense” permission model in
docs/logging_quickref.md.
Show a summary per file
| File | Description |
|---|---|
| src/workdir-setup.ts | Repairs squid log dir ownership on every run and prunes stale MCP log subdirectories. |
| src/workdir-setup.test.ts | Adds regression tests for squid ownership repair and MCP log pruning behavior. |
| docs/logging_quickref.md | Documents log directory layout, squid path asymmetry, MCP log location/pruning, and layered permissions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
✅ Contribution Check completed successfully! PR #5963 follows the applicable CONTRIBUTING.md guidelines: clear description with issue reference, tests included for functionality, documentation updated, and files are organized appropriately. |
|
🚀 Security Guard has started processing this pull request |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
🔌 Smoke Services — Service connectivity failed |
|
✅ Smoke Claude passed |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
✅ Build Test Suite completed successfully! |
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed... |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.82% | 98.82% | ➡️ +0.00% |
| Statements | 98.75% | 98.75% | ➡️ +0.00% |
| Functions | 99.72% | 99.72% | ➡️ +0.00% |
| Branches | 95.17% | 95.15% | 📉 -0.02% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
97.6% → 97.8% (+0.25%) | 97.6% → 97.8% (+0.26%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
1 similar comment
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.82% | 98.82% | ➡️ +0.00% |
| Statements | 98.75% | 98.75% | ➡️ +0.00% |
| Functions | 99.72% | 99.72% | ➡️ +0.00% |
| Branches | 95.17% | 95.15% | 📉 -0.02% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
97.6% → 97.8% (+0.25%) | 97.6% → 97.8% (+0.26%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed... |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✅ Build Test Suite completed successfully! |
|
Chroot tests failed Smoke Chroot failed - See logs for details. |
|
✅ Smoke Claude passed |
|
🚀 Security Guard has started processing this pull request |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 No GitHub write action yet; preparing required smoke-test reads and build. |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.82% | 98.82% | ➡️ +0.00% |
| Statements | 98.75% | 98.76% | 📈 +0.01% |
| Functions | 99.72% | 99.72% | ➡️ +0.00% |
| Branches | 95.17% | 95.15% | 📉 -0.02% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
97.6% → 97.8% (+0.25%) | 97.6% → 97.8% (+0.26%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test: Claude Engine Validation
Overall Result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔬 Smoke Test Results
Overall: PASS cc @lpcox Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Smoke Test: Services Connectivity
Result: FAIL — Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Copilot PAT Auth
Overall: PASS @lpcox — Auth mode: PAT (COPILOT_GITHUB_TOKEN) Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔭 Smoke Test: API Proxy OpenTelemetry Tracing
Result: All scenarios pass. OTEL tracing integration is fully functional. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS Notes
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Copilot BYOK (Direct Mode)PR: fix: harden log directory management for ARC/DinD and rootless Docker
Overall: PASS (1 data warning — pre-fetch step template substitution failed) Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
fix: harden log directory management for ARC/DinD and rootless Docker\nMerged PRs: fix: tolerate EROFS when chmod on pre-existing mcp-logs dir fails; fix: handle EPERM when chmod'ing pre-existing mcp-logs directory\nGitHub reads: ✅\nPlaywright: ✅\nFile write: ✅\nDiscussion comment: ✅\nBuild: ❌\nOverall: FAIL Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "registry.npmjs.org"See Network Configuration for more information.
|
Summary
Addresses findings from the log directory management audit (#5962). After 9 permission/ownership bug fixes landed in ~10 days for ARC/DinD support, this PR hardens the implementation and adds documentation to prevent future regressions.
Changes
Bug fix: squid-logs
onAfterEnsureinstead ofonCreatePreviously, squid-logs only got
chown(13:13)when freshly created. If the directory pre-existed from a timeout-killed run with wrong ownership (e.g.,root:rootfrom Docker daemon auto-creation), the host-side chown wouldn't fire — relying implicitly on the container entrypoint preflight.Now uses
onAfterEnsure(matching the agent-logs/session-state pattern) so ownership is repaired on every invocation. Added double-fallback: if chown fails AND chmod fails, continue silently (container Layer 2 handles it).Bug fix: retry Docker daemon probe in topology preflight
The
--network-isolationpreflight runsdocker infowith a 5-second timeout. On GitHub-hosted runners under load (concurrent service container healthchecks + image pulls during job startup), the daemon can be temporarily unresponsive, causing spurious CI failures.Now retries 3 times with 2-second delays between attempts, tolerating transient daemon backpressure. Total worst-case delay is ~19s, acceptable for a fail-stop preflight that previously caused hard job failures.
Feature: MCP log pruning
/tmp/gh-aw/mcp-logslives outsideworkDirand was never cleaned up, growing unboundedly on persistent runners. Now removes subdirectories older than 24 hours duringprepareLogDirectories().Documentation
workdir-setup.tsexplaining what each layer does, which topologies it covers, and why none should be removed.docs/logging_quickref.md: Added "Log Directory Layout" section covering:--proxy-logs-dirdirectory structureproxyLogsDir, no subdirectory)Tests
repairs squid logs ownership even when directory pre-existstolerates both chown and chmod failure on squid logs (best-effort)removes subdirectories older than 24 hours(MCP pruning)skips files (only prunes directories)tolerates unreadable directory without throwingreturns without exiting when daemon becomes reachable on retryexits when the Docker daemon is unreachable after all retriesexits when the Docker daemon probe throws on all retriesTest Results
All existing + new tests pass (29 in workdir-setup, 9 in topology, 27 in related suites).
Closes #5962