mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(agent/agentscripts): create missing log_path parent directory (#28166)
Previously, a `coder_script` whose `log_path` pointed under a directory
that did not yet exist failed before the script ran, with no per-script
log output. `OpenFile(logPath, O_CREATE|O_RDWR, 0o600)` creates the log
file but not its parent directories, so the open returned `ENOENT`. The
failure only surfaced in the agent log (`startup script(s) failed` /
`shutdown script(s) failed`) and never reached the script's own UI logs,
which made it look like a silent failure.
This creates the resolved parent directory with
`MkdirAll(filepath.Dir(logPath), 0o700)` before opening the log file, so
the script runs and its log is written. `0o700` matches the existing
script data-dir and secret-file directory conventions in this package.
Resolution of `~`, environment variables, and paths relative to `LogDir`
is unchanged; only the parent directory is now created.
Fixes coder/coder#21986
<details><summary>Implementation notes and validation</summary>
**Change**
* `agent/agentscripts/agentscripts.go`: in `(*Runner).run`, after the
full `logPath` resolution and before `OpenFile`, create the parent
directory:
```go
logDir := filepath.Dir(logPath)
if err = r.Filesystem.MkdirAll(logDir, 0o700); err != nil {
return xerrors.Errorf("create script log file directory %q: %w", logDir,
err)
}
```
**Regression test**
* `agent/agentscripts/agentscripts_test.go`:
`TestExecuteCreatesMissingLogDir` runs a script with a nested,
nonexistent `LogPath` and asserts the streamed output and that the log
file is created.
* The test uses `afero.NewOsFs()` on purpose: `afero.NewMemMapFs()`
auto-creates parent directories on `OpenFile`, so it cannot reproduce
the reported failure.
* Verified red without the fix (`open .../does/not/exist/install.log: no
such file or directory`) and green with it.
**Local validation**
* `gofmt` clean, `go vet`, `go build`, `golangci-lint run` on the
package, and `go test -race ./agent/agentscripts/` all pass.
**End-to-end**
* Validated on a dev instance with a template whose
`coder_script.log_path` targets a nested directory that does not exist.
The agent created the parents with mode `0700` and wrote the log file;
the workspace agent reported healthy.
**Prior attempts**
* [#22796](<https://github.com/coder/coder/issues/22796>) and
[#25545](<https://github.com/coder/coder/issues/25545>) proposed the
same directory-creation approach. Both were closed for non-technical
reasons (a low-effort AI PR and a stale community PR), not rejected on
the merits. This supersedes them, authored by the issue owner, using
`0o700` and adding a regression test.
</details>
---
*Raised on behalf of* @35C4n0r *by Coder Agents.*
This commit is contained in:
@@ -47,6 +47,50 @@ func TestExecuteBasic(t *testing.T) {
|
||||
require.Equal(t, "hello", log.Output)
|
||||
}
|
||||
|
||||
func TestExecuteCreatesMissingLogDir(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
fs := afero.NewOsFs()
|
||||
logger := testutil.Logger(t)
|
||||
s, err := agentssh.NewServer(context.Background(), logger, prometheus.NewRegistry(), fs, agentexec.DefaultExecer, nil)
|
||||
require.NoError(t, err)
|
||||
t.Cleanup(func() {
|
||||
_ = s.Close()
|
||||
})
|
||||
|
||||
fLogger := newFakeScriptLogger()
|
||||
runner := agentscripts.New(agentscripts.Options{
|
||||
LogDir: t.TempDir(),
|
||||
DataDirBase: t.TempDir(),
|
||||
Logger: logger,
|
||||
SSHServer: s,
|
||||
Filesystem: fs,
|
||||
GetScriptLogger: func(uuid.UUID) agentscripts.ScriptLogger {
|
||||
return fLogger
|
||||
},
|
||||
})
|
||||
defer runner.Close()
|
||||
|
||||
logPath := filepath.Join(t.TempDir(), "does", "not", "exist", "install.log")
|
||||
|
||||
aAPI := agenttest.NewFakeAgentAPI(t, logger, nil, nil)
|
||||
err = runner.Init([]codersdk.WorkspaceAgentScript{{
|
||||
LogSourceID: uuid.New(),
|
||||
LogPath: logPath,
|
||||
Script: "echo hello",
|
||||
}}, aAPI.ScriptCompleted)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, runner.Execute(ctx, agentscripts.ExecuteAllScripts))
|
||||
|
||||
log := testutil.TryReceive(ctx, t, fLogger.logs)
|
||||
require.Equal(t, "hello", log.Output)
|
||||
|
||||
exists, err := afero.Exists(fs, logPath)
|
||||
require.NoError(t, err)
|
||||
require.True(t, exists, "expected log file to be created at %s", logPath)
|
||||
}
|
||||
|
||||
func TestEnv(t *testing.T) {
|
||||
t.Parallel()
|
||||
fLogger := newFakeScriptLogger()
|
||||
|
||||
Reference in New Issue
Block a user