amadan

carlos / platform Public

layout: fix round 1 (journal durability, Validate/ReadJournal trust, comments)

Fixes five review findings against the layout plan/journal package:

- WriteJournal now writes to an O_EXCL temp file, syncs it, and links it
  to the final name instead of writing straight into an O_EXCL-opened
  final path, so a crash between open and write can no longer leave an
  empty, durable journal that recovery finds and refuses. UpdatePhase
  syncs its temp before the rename and fsyncs the journal directory
  after both operations succeed.
- Validate now refuses a journal whose db artifact is not marked
  present, and any phase other than files/row/done.
- ReadJournal refuses a journal whose content names a different host
  than the path it was read from.
- UpdatePhase assigns the new phase to the caller's journal only after
  the rename that makes it durable has succeeded, not before.
- Comments that cited plan ruling IDs and task numbers now state the
  reason directly; the spec's own codex IDs (R2-7) are untouched.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
claude 9352093c114cac82ea76e5e5d0a01ad2c9e6b3f1 parent 87ed86d
5 files changed, +243 −41
  • internal/layout/journal.go +93 −15
  • internal/layout/journal_linux_test.go +1 −1
  • internal/layout/journal_test.go +126 −6
  • internal/layout/plan.go +16 −12
  • internal/layout/plan_test.go +7 −7

CI passed — run details

diff --git a/internal/layout/journal.go b/internal/layout/journal.go
index 14eb1c961..fdb4a5344 100644
--- a/internal/layout/journal.go
+++ b/internal/layout/journal.go
@@ -59,11 +59,36 @@ const journalMax = 64 << 10
func journalDir(dataDir string) string { return filepath.Join(dataDir, "layout") }
-// WriteJournal creates the journal O_EXCL. An existing file, whoever wrote
-// it, is a refusal: the pass never overwrites a record of a move it did not
-// make.
+// syncDir fsyncs a directory. A file's own Sync makes its bytes durable,
+// but the directory entry that gives those bytes a name (a hard link, a
+// rename) is separate metadata: on ext4 with delayed allocation, a crash
+// between an otherwise-successful Link or Rename and this call can still
+// lose the name while the bytes it would have pointed to survive under
+// their old one.
+func syncDir(dir string) error {
+ d, err := os.Open(dir)
+ if err != nil {
+ return err
+ }
+ defer d.Close()
+ return d.Sync()
+}
+
+// WriteJournal creates the journal exactly once. An existing file, whoever
+// wrote it, is a refusal: the pass never overwrites a record of a move it
+// did not make.
+//
+// It writes the content to a temp file, syncs that file, then links it to
+// the final name: os.Link fails with EEXIST exactly as O_EXCL would,
+// without ever making a partly written journal visible at the final path.
+// Without the temp+sync+link, an O_EXCL create-and-write can leave an
+// empty file at the final name durable while its content is not (a power
+// loss, or a kill between OpenFile and Write). Recovery then finds a
+// journal with no content, refuses it, and the host stays fenced with
+// nothing to read to explain why.
func WriteJournal(dataDir string, j *Journal) error {
- if err := os.MkdirAll(journalDir(dataDir), 0o700); err != nil {
+ dir := journalDir(dataDir)
+ if err := os.MkdirAll(dir, 0o700); err != nil {
return err
}
if j.WrittenAt == "" {
@@ -73,18 +98,39 @@ func WriteJournal(dataDir string, j *Journal) error {
if err != nil {
return err
}
- f, err := os.OpenFile(provision.LayoutJournalPath(dataDir, j.Host), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600)
- if errors.Is(err, os.ErrExist) {
- return ErrJournalExists
- }
+ tmp, err := os.CreateTemp(dir, "."+j.Host+".tmp-*")
if err != nil {
return err
}
- if _, err := f.Write(append(raw, '\n')); err != nil {
- f.Close()
+ tmpName := tmp.Name()
+ if _, err := tmp.Write(append(raw, '\n')); err != nil {
+ tmp.Close()
+ os.Remove(tmpName)
return err
}
- return f.Close()
+ if err := tmp.Sync(); err != nil {
+ tmp.Close()
+ os.Remove(tmpName)
+ return err
+ }
+ if err := tmp.Close(); err != nil {
+ os.Remove(tmpName)
+ return err
+ }
+
+ linkErr := os.Link(tmpName, provision.LayoutJournalPath(dataDir, j.Host))
+ // The temp file's only job was to become the final name; whether that
+ // succeeded or not, its own name must not linger (nothing reads a
+ // ".tmp-" name, but a stray one left after a failed Link would look
+ // like leftover corruption to anyone auditing the directory by hand).
+ os.Remove(tmpName)
+ if linkErr != nil {
+ if errors.Is(linkErr, os.ErrExist) {
+ return ErrJournalExists
+ }
+ return linkErr
+ }
+ return syncDir(dir)
}
// ReadJournal reads the host's journal without following a symlink and
@@ -119,15 +165,22 @@ func ReadJournal(dataDir, host string) (*Journal, bool, error) {
if err := json.Unmarshal(raw, &j); err != nil {
return nil, false, fmt.Errorf("layout: %s: %w", path, err)
}
+ if j.Host != host {
+ return nil, false, fmt.Errorf("layout: %s: content names host %q, path names %q", path, j.Host, host)
+ }
return &j, true, nil
}
// UpdatePhase rewrites the journal with a new phase through a temp file and
// rename in the same directory, so a crash leaves either the old phase or
-// the new one, never a torn file.
+// the new one, never a torn file. It marshals a copy carrying the new
+// phase, not j itself, so a caller that inspects j after an error sees the
+// phase the journal on disk actually has, not one that was never made
+// durable.
func UpdatePhase(dataDir string, j *Journal, p Phase) error {
- j.Phase = p
- raw, err := json.MarshalIndent(j, "", " ")
+ next := *j
+ next.Phase = p
+ raw, err := json.MarshalIndent(&next, "", " ")
if err != nil {
return err
}
@@ -142,6 +195,15 @@ func UpdatePhase(dataDir string, j *Journal, p Phase) error {
os.Remove(name)
return err
}
+ // Synced before the rename, not after: a rename makes these bytes
+ // visible at the journal's canonical name, so they must already be
+ // durable, or a crash between the rename and a Sync afterward could
+ // leave that name pointing at content that never landed.
+ if err := tmp.Sync(); err != nil {
+ tmp.Close()
+ os.Remove(name)
+ return err
+ }
if err := tmp.Close(); err != nil {
os.Remove(name)
return err
@@ -150,7 +212,11 @@ func UpdatePhase(dataDir string, j *Journal, p Phase) error {
os.Remove(name)
return err
}
- return nil
+ // Only now, with the rename itself having succeeded, does the phase
+ // this call promised become true of both the in-memory journal and
+ // the one on disk.
+ j.Phase = p
+ return syncDir(dir)
}
// RemoveJournal deletes the host's journal. Removing an absent journal is
@@ -184,6 +250,11 @@ func Validate(j *Journal, dataDir, socketDir string, row registry.Route) (atFrom
if j.Host != row.Host {
return false, fmt.Errorf("layout: journal is for %s, row is %s", j.Host, row.Host)
}
+ switch j.Phase {
+ case PhaseFiles, PhaseRow, PhaseDone:
+ default:
+ return false, fmt.Errorf("layout: %s: journal phase %q is not files, row or done; not acting", j.Host, j.Phase)
+ }
p, err := PlanFor(dataDir, socketDir, j.Host, j.Mode)
if err != nil {
return false, err
@@ -199,6 +270,13 @@ func Validate(j *Journal, dataDir, socketDir string, row registry.Route) (atFrom
return false, fmt.Errorf("layout: %s: journal artifact %d is %q, expected %q", j.Host, i, a.Name, artifactNames[i])
}
}
+ // The db is the one artifact recovery cannot proceed without: every
+ // other artifact missing is a smaller, optional file the pass would
+ // simply skip, but a journal claiming its db was never even there
+ // describes a move that cannot have happened.
+ if !j.Artifacts[0].Present {
+ return false, fmt.Errorf("layout: %s: journal's db artifact is not marked present; not acting", j.Host)
+ }
switch (Endpoint{DB: row.DBPath, Socket: row.Socket}) {
case j.From:
return true, nil
diff --git a/internal/layout/journal_linux_test.go b/internal/layout/journal_linux_test.go
index a98d23a52..6a63551bc 100644
--- a/internal/layout/journal_linux_test.go
+++ b/internal/layout/journal_linux_test.go
@@ -13,7 +13,7 @@ import (
)
// TestReadJournalRefusesASymlinkAndAFIFO_FIFO is the FIFO half of
-// TestReadJournalRefusesASymlinkAndAFIFO (R18): /data/layout is agent-owned,
+// TestReadJournalRefusesASymlinkAndAFIFO: /data/layout is agent-owned,
// but a flat tenant shares the agent's uid during the transition and can
// plant a FIFO there in place of a journal. ReadJournal must refuse it
// rather than block on the open. Run with a timeout so a regression here
diff --git a/internal/layout/journal_test.go b/internal/layout/journal_test.go
index 4d9747499..01dea7a32 100644
--- a/internal/layout/journal_test.go
+++ b/internal/layout/journal_test.go
@@ -1,6 +1,7 @@
package layout
import (
+ "encoding/json"
"errors"
"os"
"path/filepath"
@@ -10,7 +11,9 @@ import (
)
// presence9 is a presence vector in artifactNames order: db, db-wal, db-shm,
-// env, sidecar-env, config-env, socket, litestream-shadow, corrupt (R5).
+// env, sidecar-env, config-env, socket, litestream-shadow, corrupt. The db
+// is marked present, since Validate now requires that of any journal it
+// accepts.
var presence9 = []bool{true, false, false, true, false, true, false, false, false}
func journalFor(t *testing.T, dd string, mode Mode) *Journal {
@@ -50,6 +53,65 @@ func TestJournalWriteIsExclusiveAndPhasesAdvance(t *testing.T) {
}
}
+// tempFilesIn lists the WriteJournal/UpdatePhase temp names (".<host>.tmp-*")
+// left in dir, so a test can assert none survive a call.
+func tempFilesIn(t *testing.T, dir string) []string {
+ t.Helper()
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var names []string
+ for _, e := range entries {
+ if filepath.Ext(e.Name()) != ".json" {
+ names = append(names, e.Name())
+ }
+ }
+ return names
+}
+
+// TestWriteJournalRefusesAPreexistingFinalPath pins that the link-based
+// create refuses exactly like the O_EXCL it replaces, even when the file
+// at the final path was never written by WriteJournal itself (a hand
+// dropped file, or debris from something else): os.Link cannot overwrite
+// an existing name any more than O_EXCL can, so the caller can trust
+// ErrJournalExists means "something is already there" regardless of how it
+// got there.
+func TestWriteJournalRefusesAPreexistingFinalPath(t *testing.T) {
+ dd := t.TempDir()
+ dir := filepath.Join(dd, "layout")
+ if err := os.MkdirAll(dir, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ final := filepath.Join(dir, "h.example.com.json")
+ if err := os.WriteFile(final, []byte("not a journal"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ j := journalFor(t, dd, Mode("nested"))
+ if err := WriteJournal(dd, j); !errors.Is(err, ErrJournalExists) {
+ t.Fatalf("WriteJournal over a pre-existing file = %v, want ErrJournalExists", err)
+ }
+ if got := tempFilesIn(t, dir); len(got) != 0 {
+ t.Errorf("temp files left behind after a refused write: %v", got)
+ }
+}
+
+// TestWriteJournalLeavesNoTempOnSuccess pins the other half of the same
+// contract: a successful write's temp name must not survive either, since
+// the whole point of linking rather than renaming in place was to prove
+// the final name's content durable before anything reads it, not to leave
+// a second, forgotten copy sitting beside it.
+func TestWriteJournalLeavesNoTempOnSuccess(t *testing.T) {
+ dd := t.TempDir()
+ j := journalFor(t, dd, Mode("nested"))
+ if err := WriteJournal(dd, j); err != nil {
+ t.Fatal(err)
+ }
+ if got := tempFilesIn(t, journalDir(dd)); len(got) != 0 {
+ t.Errorf("temp files left behind after a successful write: %v", got)
+ }
+}
+
func TestReadJournalRefusesASymlinkAndAFIFO(t *testing.T) {
dd := t.TempDir()
dir := filepath.Join(dd, "layout")
@@ -64,6 +126,31 @@ func TestReadJournalRefusesASymlinkAndAFIFO(t *testing.T) {
}
}
+// TestReadJournalRefusesAHostMismatch pins that ReadJournal checks the
+// journal's own content against the path it was read from: a file whose
+// content names a different host than the file name itself is a sign
+// something replaced it (a copy, a stale hard link, a bug in whatever
+// wrote it) and must not be trusted for that host's recovery.
+func TestReadJournalRefusesAHostMismatch(t *testing.T) {
+ dd := t.TempDir()
+ dir := filepath.Join(dd, "layout")
+ if err := os.MkdirAll(dir, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ j := journalFor(t, dd, Mode("nested"))
+ j.Host = "other.example.com"
+ raw, err := json.Marshal(j)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := os.WriteFile(filepath.Join(dir, "h.example.com.json"), raw, 0o600); err != nil {
+ t.Fatal(err)
+ }
+ if _, _, err := ReadJournal(dd, "h.example.com"); err == nil {
+ t.Error("a journal whose content names a different host than its path was read")
+ }
+}
+
func TestValidateChecksBothEndpointsFromTheJournalsOwnDirection(t *testing.T) {
dd := t.TempDir()
j := journalFor(t, dd, Mode("nested"))
@@ -97,10 +184,9 @@ func TestValidateChecksBothEndpointsFromTheJournalsOwnDirection(t *testing.T) {
}
}
-// TestValidateRefusesAWrongArtifactCount pins R5's "Validate's length check
-// becomes 9": a journal that predates the two new artifacts (or one that is
-// simply malformed) must not be treated as valid just because its endpoints
-// happen to match.
+// TestValidateRefusesAWrongArtifactCount: a journal that predates the two
+// newest artifacts (or one that is simply malformed) must not be treated
+// as valid just because its endpoints happen to match.
func TestValidateRefusesAWrongArtifactCount(t *testing.T) {
dd := t.TempDir()
j := journalFor(t, dd, Mode("nested"))
@@ -108,7 +194,41 @@ func TestValidateRefusesAWrongArtifactCount(t *testing.T) {
short.Artifacts = j.Artifacts[:7]
row := registry.Route{Host: "h.example.com", DBPath: j.From.DB, Socket: j.From.Socket}
if _, err := Validate(&short, dd, "/run/carlos", row); err == nil {
- t.Error("a journal with 7 artifacts (pre-R5) was accepted")
+ t.Error("a journal with only 7 of the 9 artifacts was accepted")
+ }
+}
+
+// TestValidateRequiresTheDBArtifactPresent pins that a forged journal
+// cannot claim its db was never there and still be accepted: everything
+// else in an optional artifact list can be absent and the pass simply
+// skips it, but a journal with no db describes a move that cannot have
+// happened, whatever its endpoints say.
+func TestValidateRequiresTheDBArtifactPresent(t *testing.T) {
+ dd := t.TempDir()
+ j := journalFor(t, dd, Mode("nested"))
+ row := registry.Route{Host: "h.example.com", DBPath: j.From.DB, Socket: j.From.Socket}
+ forged := *j
+ forged.Artifacts = append([]JournalArtifact(nil), j.Artifacts...)
+ forged.Artifacts[0].Present = false
+ if _, err := Validate(&forged, dd, "/run/carlos", row); err == nil {
+ t.Error("a journal whose db artifact is not present was accepted")
+ }
+}
+
+// TestValidateRefusesAnUndefinedPhase pins that Validate only accepts the
+// three phases the pass itself ever writes: an empty or unrecognized phase
+// is a sign of a forged or corrupted journal, not one from an earlier
+// build that Validate should try to interpret.
+func TestValidateRefusesAnUndefinedPhase(t *testing.T) {
+ dd := t.TempDir()
+ j := journalFor(t, dd, Mode("nested"))
+ row := registry.Route{Host: "h.example.com", DBPath: j.From.DB, Socket: j.From.Socket}
+ for _, phase := range []Phase{"", "bogus"} {
+ forged := *j
+ forged.Phase = phase
+ if _, err := Validate(&forged, dd, "/run/carlos", row); err == nil {
+ t.Errorf("phase %q was accepted", phase)
+ }
}
}
diff --git a/internal/layout/plan.go b/internal/layout/plan.go
index bfee4b2d4..7ede6a611 100644
--- a/internal/layout/plan.go
+++ b/internal/layout/plan.go
@@ -66,18 +66,20 @@ type Plan struct {
Artifacts []Artifact
}
-// artifactNames is the fixed order of the nine artifacts (R5 appended
-// litestream-shadow and corrupt after socket, so the first seven indices
-// keep their original meaning). The journal records presence by this
-// order, so it is part of the on-disk contract.
+// artifactNames is the fixed order of the nine artifacts the pass tracks.
+// litestream-shadow and corrupt sit after socket so that an index into an
+// existing Artifacts slice (Artifacts[0] for the db, and so on) keeps its
+// meaning. The journal records presence by this order, so it is part of
+// the on-disk contract.
var artifactNames = []string{
"db", "db-wal", "db-shm", "env", "sidecar-env", "config-env", "socket",
"litestream-shadow", "corrupt",
}
// rawInstanceDir and rawRunDir are the ONLY places the platform names the
-// raw directories behind the two bind mounts; a grep test (task 3) pins
-// that.
+// raw directories behind the two bind mounts, so a grep test can pin that
+// and catch any new hand-built path that starts reading or writing through
+// the bind mount instead.
func rawInstanceDir(dataDir, host string) string {
return filepath.Join(dataDir, ".instances", provision.InstanceID(host))
}
@@ -88,9 +90,11 @@ func rawRunDir(socketDir, host string) string {
// shadowName is the litestream shadow directory's file name for host,
// derived exactly as internal/lease/lease.go's newStore does for a
-// database path whose base name is "<host>.db" (that function is
-// unexported and lives in a package this one must not import back from,
-// since internal/activator will import this package in task 3).
+// database path whose base name is "<host>.db". That derivation is on an
+// unexported helper in internal/lease, so it is repeated here rather than
+// imported: this package has no other reason to depend on litestream's
+// setup, and a two-line string format is cheaper to keep in step by hand
+// than a new cross-package dependency is to justify.
func shadowName(host string) string {
return ".activator-" + host + ".db-litestream"
}
@@ -99,7 +103,7 @@ func shadowName(host string) string {
// in artifactNames order. The nested set is expressed through the raw
// directories, except for the socket, which the pass never renames and so
// is expressed through the mount-visible ("instances", not ".instances")
-// path the row itself will carry (R8).
+// path the row itself will carry.
func flatFiles(dataDir, socketDir, host string) []string {
db := provision.DefaultDBPath(dataDir, host)
return []string{
@@ -137,8 +141,8 @@ func nestedFiles(dataDir, socketDir, host string) []string {
// directory) it will rename.
//
// Both socket endpoints are derived from socketDir, never from the
-// compiled-in provision.SocketDir/DefaultSocket/NestedSocket (R8): those
-// ignore any directory a caller passes, which is invisible in production
+// compiled-in provision.SocketDir/DefaultSocket/NestedSocket: those ignore
+// any directory a caller passes, which is invisible in production
// (socketDir is always provision.SocketDir there) but would silently break
// every test that wants an isolated socket tree.
func PlanFor(dataDir, socketDir, host string, mode Mode) (*Plan, error) {
diff --git a/internal/layout/plan_test.go b/internal/layout/plan_test.go
index 973b84069..27381c460 100644
--- a/internal/layout/plan_test.go
+++ b/internal/layout/plan_test.go
@@ -49,7 +49,7 @@ func TestPlanForNestedDerivesEveryPathFromTheHost(t *testing.T) {
t.Error("a destination through the bind mount would EXDEV")
}
- // R5: the two artifacts appended after socket.
+ // The two artifacts appended after socket.
if byName["litestream-shadow"].Src != "/data/.activator-h.example.com.db-litestream" {
t.Errorf("litestream-shadow src = %q", byName["litestream-shadow"].Src)
}
@@ -72,7 +72,7 @@ func TestPlanForNestedDerivesEveryPathFromTheHost(t *testing.T) {
t.Errorf("corrupt artifact = %+v, want a non-directory optional file", byName["corrupt"])
}
- // R8: both socket endpoints come from the socketDir argument, and the
+ // Both socket endpoints come from the socketDir argument, and the
// nested one sits under "instances" (the mount-visible name), not the
// raw dotted directory: nothing is ever renamed for the socket.
if byName["socket"].Src != "/run/carlos/h.example.com.sock" {
@@ -86,7 +86,7 @@ func TestPlanForNestedDerivesEveryPathFromTheHost(t *testing.T) {
}
}
-// TestPlanForUsesTheGivenSocketDirNotTheCompiledDefault pins R8: PlanFor
+// TestPlanForUsesTheGivenSocketDirNotTheCompiledDefault pins that PlanFor
// must derive both socket endpoints from its socketDir argument, never from
// the compiled-in provision.SocketDir or provision.DefaultSocket/NestedSocket
// (which ignore the argument entirely). A test that only ever passes
@@ -138,10 +138,10 @@ func TestPlanForFlatIsTheExactReverse(t *testing.T) {
}
// TestRawRunDirIsTheDottedRawSocketTree pins rawRunDir's own doc comment
-// ("<socketDir>/.instances/<iid>"): task 3's EnsureDirs creates the nested
-// run directory through this raw path even though the socket file inside
-// it is never renamed (PlanFor's socket artifact records the mount-visible
-// path instead, R8).
+// ("<socketDir>/.instances/<iid>"): a later pass creates the nested run
+// directory through this raw path even though the socket file inside it is
+// never renamed (PlanFor's socket artifact records the mount-visible path
+// instead).
func TestRawRunDirIsTheDottedRawSocketTree(t *testing.T) {
got := rawRunDir("/run/carlos", "h.example.com")
want := filepath.Join("/run/carlos/.instances", provision.InstanceID("h.example.com"))