diff --git a/docs/WINDOWS_OBSERVATION_DIAGNOSTICS.md b/docs/WINDOWS_OBSERVATION_DIAGNOSTICS.md index 55eeef9e3..b4a917d8a 100644 --- a/docs/WINDOWS_OBSERVATION_DIAGNOSTICS.md +++ b/docs/WINDOWS_OBSERVATION_DIAGNOSTICS.md @@ -1,6 +1,8 @@ # Windows observation-cache diagnostics -Observation claims keep the 250ms production budget and fail closed. A +Observation claims fail closed. The 250ms budget covers the locked read/publication +transaction after root and private temporary-file preflight; the original caller +context covers both phases. Complete claim duration can exceed 250ms. A `cache_unavailable` result does not establish whether the cause was a deadline, private-file validation or a Windows I/O error. It also does not prove that an attempted bit was not published. Never retry an uncertain claim automatically. @@ -9,12 +11,12 @@ The public error message and notification receipts remain `cache_unavailable`. Internally, declare `var failure *observation.ClaimFailure` and call `errors.As(err, &failure)` to inspect bounded details: -- `Phase`: validation, path/root preparation, lock, clock, read/decode, +- `Phase`: validation, path/root preparation, temp preparation, lock, clock, read/decode, record/encode, publication or the check after publication. - `Class` and `OSCode`: sanitized failure category and numeric OS error. Windows sharing violation (32) and lock violation (33) are distinct. - `TotalElapsed`, `StageElapsed` and `BudgetElapsed`: elapsed durations. - Root/path preflight precedes the existing claim timer, so total duration can + Root/path and private temporary-file preflight precede the claim timer, so total duration can exceed budget duration. `BudgetState` records the timer independently of the operation's error; an I/O error and an expired deadline can coexist. - `MayHavePublished`: conservative uncertainty once publication starts. @@ -82,3 +84,21 @@ not cancel existing sessions, retry tests, change antivirus settings or install tools. Unlike sanitized claim diagnostics, the raw OS trace includes paths and process metadata from the disposable test runner. Do not run it on a real user profile or infer a driver cause from a successful control recording. + +## Preflight and transaction boundary + +Each claim allocates one private empty temporary file before starting its 250ms +transaction context. The caller context is checked before and after allocation; +it is never refreshed. Preflight does not inspect or change attempt history. +Locking, current document/clock decisions, writing, target validation, replacing +rename and checked close remain in the transaction. The pre-rename and final +deadline checks still deny expired or uncertain publication. + +Unused preparations are deleted and closed on duplicate, lock, read, decode or +cancellation exits. A duplicate now also requires a writable private cache root: +creation failure returns `cache_unavailable` before duplicate detection. This is +a conservative admission failure, never permission to deliver or retry. + +This boundary change addresses the attributed temporary creation stalls without +claiming they are faster. Other synchronous I/O can still exhaust the transaction +or caller deadline. No first-valid assertion, fixed window or private ACL is relaxed. diff --git a/docs/gemini-notifications.md b/docs/gemini-notifications.md index 15983bc7a..3b3d50588 100644 --- a/docs/gemini-notifications.md +++ b/docs/gemini-notifications.md @@ -85,7 +85,9 @@ Duplicate suppression records attempts per channel for a 60-second window, with at most 256 hashed observation markers. It does not guarantee successful delivery or exactly-once notification display. Missing native timestamps are scoped to one invocation. An unsafe, unavailable or slow cache suppresses the affected attempt; -the cache claim budget is 250 ms and there is no automatic delivery retry. A crash +the locked cache transaction budget is 250 ms after root and private temporary-file +preflight. The caller deadline also applies to preflight, so complete claim duration +can exceed 250 ms. There is no automatic delivery retry. A crash or cache loss can permit duplicates outside this bounded guarantee. Restart Gemini after setup or update so its effective hooks are reloaded. Use the diff --git a/internal/installruntime/private_cache_windows.go b/internal/installruntime/private_cache_windows.go index 33581ee7f..c0739ce9a 100644 --- a/internal/installruntime/private_cache_windows.go +++ b/internal/installruntime/private_cache_windows.go @@ -145,82 +145,119 @@ func (r *PrivateCacheRoot) Write(name string, data []byte) error { return r.WriteContext(context.Background(), name, data) } -// WriteContext checks cancellation immediately before replacing the document. -// Synchronous filesystem calls and a rename already in flight cannot be canceled; -// callers must still check their deadline after publication before delivery. -func (r *PrivateCacheRoot) WriteContext(ctx context.Context, name string, data []byte) (err error) { +// WriteContext prepares and publishes under the supplied context. Claim callers +// may instead prepare before their transaction budget starts. +func (r *PrivateCacheRoot) WriteContext(ctx context.Context, name string, data []byte) error { if len(data) == 0 { return fmt.Errorf("private cache write requires bytes") } + prepared, err := r.PrepareWrite(ctx, name) + if err != nil { + return err + } + return prepared.WriteContext(ctx, data) +} + +// PrivateCacheWrite owns one empty private temporary file. It is single-use and +// must be closed if the caller decides not to publish. It never owns the root. +type PrivateCacheWrite struct { + root *PrivateCacheRoot + name string + file *os.File + ctx context.Context + published bool +} + +// PrepareWrite allocates without reading or modifying attempt history. The +// caller context governs preflight; synchronous creation cannot be interrupted. +func (r *PrivateCacheRoot) PrepareWrite(ctx context.Context, name string) (*PrivateCacheWrite, error) { + if err := ctx.Err(); err != nil { + return nil, err + } if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `\/:`) { - return fmt.Errorf("invalid relative Windows component") + return nil, fmt.Errorf("invalid relative Windows component") } var parent windows.Handle - trace.WithRegion(ctx, "observation.publish/root", func() { - parent, err = r.parent() - }) + var err error + trace.WithRegion(ctx, "observation.publish/root", func() { parent, err = r.parent() }) if err != nil { - return err + return nil, err } var random [16]byte - trace.WithRegion(ctx, "observation.publish/random", func() { - _, err = rand.Read(random[:]) - }) + trace.WithRegion(ctx, "observation.publish/random", func() { _, err = rand.Read(random[:]) }) if err != nil { - return err + return nil, err } temp := ".observations-" + hex.EncodeToString(random[:]) var handle windows.Handle - trace.WithRegion(ctx, "observation.publish/create", func() { - handle, err = windowsCreatePrivateCacheAt(parent, temp) - }) + trace.WithRegion(ctx, "observation.publish/create", func() { handle, err = windowsCreatePrivateCacheAt(parent, temp) }) if err != nil { - return err + return nil, err + } + prepared := &PrivateCacheWrite{root: r, name: name, file: os.NewFile(uintptr(handle), temp), ctx: ctx} + if err := ctx.Err(); err != nil { + _ = prepared.Close() + return nil, err } - f := os.NewFile(uintptr(handle), temp) - published := false + return prepared, nil +} + +// Close removes an unused temp and releases its handle. Publication checks this +// close before granting admission; deferred claim cleanup is idempotent. +func (w *PrivateCacheWrite) Close() error { + if w == nil || w.file == nil { + return nil + } + f := w.file + w.file = nil + if !w.published { + trace.WithRegion(w.ctx, "observation.publish/cleanup", func() { _ = windowsDeleteHandle(windows.Handle(f.Fd())) }) + } + var err error + trace.WithRegion(w.ctx, "observation.publish/close", func() { err = f.Close() }) + return err +} + +// WriteContext publishes once, revalidating root and target under the caller's +// lock. Its checked close and pre-rename deadline guard precede admission. +func (w *PrivateCacheWrite) WriteContext(ctx context.Context, data []byte) (err error) { + if w == nil || w.file == nil { + return fmt.Errorf("private cache preparation is closed") + } + w.ctx = ctx defer func() { - if !published { - trace.WithRegion(ctx, "observation.publish/cleanup", func() { - _ = windowsDeleteHandle(handle) - }) - } - var closeErr error - trace.WithRegion(ctx, "observation.publish/close", func() { - closeErr = f.Close() - }) - if err == nil { + if closeErr := w.Close(); err == nil { err = closeErr } }() + if len(data) == 0 { + return fmt.Errorf("private cache write requires bytes") + } + var parent windows.Handle + trace.WithRegion(ctx, "observation.publish/root", func() { parent, err = w.root.parent() }) + if err != nil { + return err + } var written int - trace.WithRegion(ctx, "observation.publish/write", func() { - written, err = f.Write(data) - }) + trace.WithRegion(ctx, "observation.publish/write", func() { written, err = w.file.Write(data) }) if err != nil { return err } if written != len(data) { return io.ErrShortWrite } - trace.WithRegion(ctx, "observation.publish/target", func() { - err = validatePrivateCacheReplacement(parent, name) - }) + trace.WithRegion(ctx, "observation.publish/target", func() { err = validatePrivateCacheReplacement(parent, w.name) }) if err != nil { return err } if err = ctx.Err(); err != nil { return err } - trace.WithRegion(ctx, "observation.publish/rename", func() { - err = windowsRenameHandle(handle, parent, name, true) - }) + trace.WithRegion(ctx, "observation.publish/rename", func() { err = windowsRenameHandle(windows.Handle(w.file.Fd()), parent, w.name, true) }) if err != nil { return err } - published = true - // A close error still denies the claim, even if its attempted bit has already - // been published. The caller also checks its deadline before granting delivery. + w.published = true return nil } diff --git a/internal/installruntime/private_cache_windows_test.go b/internal/installruntime/private_cache_windows_test.go index 290506677..5f13bdb19 100644 --- a/internal/installruntime/private_cache_windows_test.go +++ b/internal/installruntime/private_cache_windows_test.go @@ -126,6 +126,11 @@ func TestWindowsPrivateCacheRootRechecksACL(t *testing.T) { t.Fatal(err) } defer session.Close() + prepared, err := session.PrepareWrite(context.Background(), "observations.json") + if err != nil { + t.Fatal(err) + } + defer func() { _ = prepared.Close() }() original, err := windows.GetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) if err != nil { t.Fatal(err) @@ -157,7 +162,7 @@ func TestWindowsPrivateCacheRootRechecksACL(t *testing.T) { if _, err := session.Read("observations.json", 100); err == nil { t.Fatal("changed root ACL permitted read") } - if err := session.Write("observations.json", []byte("next")); err == nil { + if err := prepared.WriteContext(context.Background(), []byte("next")); err == nil { t.Fatal("changed root ACL permitted publication") } data, err := os.ReadFile(filepath.Join(root, "observations.json")) @@ -166,6 +171,39 @@ func TestWindowsPrivateCacheRootRechecksACL(t *testing.T) { } } +// Red condition: a consumed preparation permits a second replacement or leaves +// its temporary file behind after the checked publication close. +func TestWindowsPreparedCacheWriteIsSingleUse(t *testing.T) { + root := t.TempDir() + if err := RestrictPrivatePath(root); err != nil { + t.Fatal(err) + } + session, err := OpenPrivateCacheRoot(root) + if err != nil { + t.Fatal(err) + } + defer session.Close() + prepared, err := session.PrepareWrite(context.Background(), "observations.json") + if err != nil { + t.Fatal(err) + } + defer func() { _ = prepared.Close() }() + if err := prepared.WriteContext(context.Background(), []byte("first")); err != nil { + t.Fatal(err) + } + if err := prepared.WriteContext(context.Background(), []byte("second")); err == nil { + t.Fatal("consumed preparation published twice") + } + data, err := session.Read("observations.json", 100) + if err != nil || !bytes.Equal(data, []byte("first")) { + t.Fatalf("second write changed document: %q/%v", data, err) + } + entries, err := os.ReadDir(root) + if err != nil || len(entries) != 1 || entries[0].Name() != "observations.json" { + t.Fatalf("publication left temp: %v/%v", entries, err) + } +} + // Diagnostic only: compare the same locked read/publication workload, including // initial root validation in both paths. No machine-dependent timing gate. func BenchmarkWindowsPrivateCacheRoot(b *testing.B) { diff --git a/internal/notification/observation/cache.go b/internal/notification/observation/cache.go index f347e814d..16a51a700 100644 --- a/internal/notification/observation/cache.go +++ b/internal/notification/observation/cache.go @@ -16,6 +16,9 @@ import ( const cacheLimit = 256 const cacheBytes = 48 * 1024 const cacheWindow = 60.0 + +// ClaimBudget bounds the locked read/publication transaction after preflight. +// The caller context also governs root and private temporary-file preparation. const ClaimBudget = 250 * time.Millisecond // RecentCache stores attempts, not successful deliveries or replayable events. @@ -59,6 +62,10 @@ func (c *RecentCache) Claim(ctx context.Context, key string, bit uint8) (bool, e return false, diag.fail(err, "validation") } defer cache.Close() + diag.enter("prepare") + if err = cache.Prepare(ctx); err != nil { + return false, diag.fail(err, "validation") + } lockCtx, cancel := context.WithTimeout(ctx, ClaimBudget) defer cancel() diag.budget, diag.budgetStarted = lockCtx, time.Now() diff --git a/internal/notification/observation/cache_other.go b/internal/notification/observation/cache_other.go index a3911fad8..132fb1ca6 100644 --- a/internal/notification/observation/cache_other.go +++ b/internal/notification/observation/cache_other.go @@ -19,6 +19,7 @@ func openCache(root string) (*pathCache, error) { } return &pathCache{root: root}, nil } +func (c *pathCache) Prepare(context.Context) error { return errors.New("cache_unavailable") } func (c *pathCache) Read() ([]byte, error) { return readCache(c.root) } func (c *pathCache) Write(_ context.Context, data []byte) error { return writeCache(c.root, data) } func (c *pathCache) Close() {} diff --git a/internal/notification/observation/cache_preflight_test.go b/internal/notification/observation/cache_preflight_test.go new file mode 100644 index 000000000..208a5d9b5 --- /dev/null +++ b/internal/notification/observation/cache_preflight_test.go @@ -0,0 +1,72 @@ +//go:build linux || darwin || windows + +package observation + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" +) + +// Red condition: a duplicate claim burns another bit or leaves its eager temp. +func TestDuplicateClaimCleansPreparation(t *testing.T) { + c, key := cacheFixture(t) + if got, err := c.Claim(context.Background(), key, 1); !got || err != nil { + t.Fatalf("seed: %v/%v", got, err) + } + path := filepath.Join(c.Root, "observations.json") + before, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if got, err := c.Claim(context.Background(), key, 1); got || err != nil { + t.Fatalf("duplicate: %v/%v", got, err) + } + after, err := os.ReadFile(path) + if err != nil || !bytes.Equal(before, after) { + t.Fatalf("duplicate changed history: %q/%v", after, err) + } + entries, err := os.ReadDir(c.Root) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if entry.Name() != "observations.json" && entry.Name() != ".observations.lock" { + t.Fatalf("duplicate left temp: %s", entry.Name()) + } + } +} + +// Red condition: canceled preflight creates files or enters a transaction. +func TestCanceledClaimDoesNotPrepare(t *testing.T) { + for _, expired := range []bool{false, true} { + name := "canceled" + if expired { + name = "expired" + } + t.Run(name, func(t *testing.T) { + c, key := cacheFixture(t) + ctx, cancel := context.WithCancel(context.Background()) + want := "canceled" + cancel() + if expired { + ctx, cancel = context.WithDeadline(context.Background(), time.Unix(1, 0)) + want = "deadline" + } + defer cancel() + got, err := c.Claim(ctx, key, 1) + var failure *ClaimFailure + if got || !errors.As(err, &failure) || failure.Phase != "prepare" || failure.Class != want || failure.BudgetState != "not_started" || failure.MayHavePublished { + t.Fatalf("canceled preflight: %v/%#v", got, err) + } + entries, err := os.ReadDir(c.Root) + if err != nil || len(entries) != 0 { + t.Fatalf("canceled preflight left files: %v/%v", entries, err) + } + }) + } +} diff --git a/internal/notification/observation/cache_preservation_windows_test.go b/internal/notification/observation/cache_preservation_windows_test.go index 28c44234a..bc346f246 100644 --- a/internal/notification/observation/cache_preservation_windows_test.go +++ b/internal/notification/observation/cache_preservation_windows_test.go @@ -109,8 +109,8 @@ func TestWindowsCacheFailedPreparationPreservesAttempt(t *testing.T) { t.Fatalf("failed preparation granted delivery: %v/%v", claimed, claimErr) } // The real FILE_ADD_FILE denial must be distinguished from an unrelated - // timeout. Publication uncertainty stays conservative even on this failure. - if failure.Phase != "publish" || failure.Class != "permission" || failure.OSCode != uint32(windows.ERROR_ACCESS_DENIED) || !failure.MayHavePublished { + // timeout. Preflight cannot have published and has not started the transaction. + if failure.Phase != "prepare" || failure.Class != "permission" || failure.OSCode != uint32(windows.ERROR_ACCESS_DENIED) || failure.MayHavePublished || failure.BudgetState != "not_started" { t.Fatalf("preparation failure diagnostics: %#v", failure) } got, err := os.ReadFile(path) diff --git a/internal/notification/observation/cache_publication_test.go b/internal/notification/observation/cache_publication_test.go index fa2c2bfb3..bb67af8af 100644 --- a/internal/notification/observation/cache_publication_test.go +++ b/internal/notification/observation/cache_publication_test.go @@ -36,6 +36,9 @@ func TestCacheCanceledPublicationPreservesDocument(t *testing.T) { t.Fatal(err) } defer cache.Close() + if err := cache.Prepare(context.Background()); err != nil { + t.Fatal(err) + } ctx, cancel := context.WithCancel(context.Background()) want := context.Canceled if deadline { diff --git a/internal/notification/observation/cache_test.go b/internal/notification/observation/cache_test.go index 49935b2a4..bc3024308 100644 --- a/internal/notification/observation/cache_test.go +++ b/internal/notification/observation/cache_test.go @@ -266,11 +266,11 @@ func TestUnavailableCacheAndLockBudgetAreNeutral(t *testing.T) { start := time.Now() got, err := c.Claim(ctx, key, 1) release() - if got || err == nil || time.Since(start) > 500*time.Millisecond { + if got || err == nil { t.Fatalf("lock was unbounded or admitted effect: %v/%v after %s", got, err, time.Since(start)) } var failure *ClaimFailure - if !errors.As(err, &failure) || failure.Phase != "lock" || failure.Class != "deadline" || failure.BudgetState != "deadline" || failure.MayHavePublished { + if !errors.As(err, &failure) || failure.Phase != "lock" || failure.Class != "deadline" || failure.BudgetState != "deadline" || failure.MayHavePublished || failure.BudgetElapsed > 500*time.Millisecond { t.Fatalf("held lock diagnostics lost the admission boundary: %#v", err) } if err = os.WriteFile(filepath.Join(c.Root, "observations.json"), []byte("PRIVATE_CORRUPT_CACHE"), 0600); err != nil { diff --git a/internal/notification/observation/cache_unix.go b/internal/notification/observation/cache_unix.go index ba3e7cfe4..8a3ea7fbd 100644 --- a/internal/notification/observation/cache_unix.go +++ b/internal/notification/observation/cache_unix.go @@ -59,20 +59,67 @@ func writeCache(root string, data []byte) error { } func writeCacheContext(ctx context.Context, root string, data []byte) error { - path := filepath.Join(root, ".observations-"+uuid.NewString()) + cache := &pathCache{root: root} + defer cache.Close() + if err := cache.Prepare(ctx); err != nil { + return err + } + return cache.Write(ctx, data) +} + +type pathCache struct { + root string + temp string + file *os.File +} + +func openCache(root string) (*pathCache, error) { + if err := checkCacheRoot(root); err != nil { + return nil, err + } + return &pathCache{root: root}, nil +} +func (c *pathCache) Read() ([]byte, error) { return readCache(c.root) } +func (c *pathCache) Prepare(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return err + } + path := filepath.Join(c.root, ".observations-"+uuid.NewString()) f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return err } - defer func() { _ = os.Remove(path) }() - n, err := f.Write(data) + c.temp, c.file = path, f + if err := ctx.Err(); err != nil { + c.Close() + return err + } + return nil +} +func (c *pathCache) Close() { + if c.file != nil { + _ = c.file.Close() + c.file = nil + } + if c.temp != "" { + _ = os.Remove(c.temp) + c.temp = "" + } +} +func (c *pathCache) Write(ctx context.Context, data []byte) error { + if c.file == nil { + return errors.New("cache preparation is closed") + } + defer c.Close() + n, err := c.file.Write(data) if err == nil && n != len(data) { err = io.ErrShortWrite } if err == nil { - err = f.Sync() + err = c.file.Sync() } - closeErr := f.Close() + closeErr := c.file.Close() + c.file = nil if err != nil { return err } @@ -82,19 +129,9 @@ func writeCacheContext(ctx context.Context, root string, data []byte) error { if err := ctx.Err(); err != nil { return err } - return os.Rename(path, filepath.Join(root, "observations.json")) -} - -type pathCache struct{ root string } - -func openCache(root string) (*pathCache, error) { - if err := checkCacheRoot(root); err != nil { - return nil, err + if err := os.Rename(c.temp, filepath.Join(c.root, "observations.json")); err != nil { + return err } - return &pathCache{root: root}, nil -} -func (c *pathCache) Read() ([]byte, error) { return readCache(c.root) } -func (c *pathCache) Write(ctx context.Context, data []byte) error { - return writeCacheContext(ctx, c.root, data) + c.temp = "" + return nil } -func (c *pathCache) Close() {} diff --git a/internal/notification/observation/cache_windows.go b/internal/notification/observation/cache_windows.go index c939e92b2..e8485f5b7 100644 --- a/internal/notification/observation/cache_windows.go +++ b/internal/notification/observation/cache_windows.go @@ -17,7 +17,8 @@ func writeCache(root string, data []byte) error { } type windowsCache struct { - root *installruntime.PrivateCacheRoot + root *installruntime.PrivateCacheRoot + prepared *installruntime.PrivateCacheWrite } func openCache(root string) (*windowsCache, error) { @@ -28,7 +29,15 @@ func openCache(root string) (*windowsCache, error) { return &windowsCache{root: session}, nil } func (c *windowsCache) Read() ([]byte, error) { return c.root.Read("observations.json", cacheBytes) } +func (c *windowsCache) Prepare(ctx context.Context) error { + var err error + c.prepared, err = c.root.PrepareWrite(ctx, "observations.json") + return err +} func (c *windowsCache) Write(ctx context.Context, data []byte) error { - return c.root.WriteContext(ctx, "observations.json", data) + return c.prepared.WriteContext(ctx, data) +} +func (c *windowsCache) Close() { + _ = c.prepared.Close() + c.root.Close() } -func (c *windowsCache) Close() { c.root.Close() }