Skip to content

Preserve stable params lock ownership - #116

Merged
pfeiferj merged 3 commits into
pfeiferj:mainfrom
FrogAi:codex/stabilize-params-locking
Sep 7, 2026
Merged

Preserve stable params lock ownership#116
pfeiferj merged 3 commits into
pfeiferj:mainfrom
FrogAi:codex/stabilize-params-locking

Conversation

@FrogAi

@FrogAi FrogAi commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Keep the shared params .lock inode in place after writes and removals. Unlinking that pathname lets a second process acquire a new lock file while an existing process still owns the old inode, defeating cross-process exclusion.

Close owned value/directory handles, preserve the existing lock permissions and bounded acquisition policy, and report meaningful close errors. PutParam closes and checks its temporary file before publication. Both operations close the directory after syncing it and return a close error when no earlier sync error takes precedence.

Contract Result
Another process holds the existing lock inode Acquisition exhausts the existing policy; protected bytes stay unchanged
Successful put/remove Existing lock inode persists; owned descriptors close
Temporary Close reports EIO after Sync Return the close error before Rename; previously published bytes remain
Directory Sync and/or Close reports an error Close once; return Sync's error first, otherwise Close's error

The close-error evidence models Go API return values after a real close. It does not demonstrate corrupt replacement data, physical disk failure, crash durability, or device incidence.

Ownership, error ordering and complete fault reproducer

The successful write sequence is Write → temporary Sync → temporary Close → lock → Rename → directory Sync → directory Close → unlock. Temporary-file cleanup remains deferred for earlier Write/Sync errors; ownership clears immediately after explicit Close even when it reports an error. Directory handles have only Sync then Close before return, so direct calls and preserved error precedence suffice without extra deferred ownership state. No raw close syscall is retried.

The lock is created with mode 0775 subject to umask; existing mode/inode are retained. Acquisition remains at most 51 attempts separated by 50 one-millisecond sleeps, not a wall-clock guarantee. RemoveParam ignores the unlink result.

Tested source and validation scope

  • Upstream comparison: 7201c6b4b4ec1b0b9ea21daa8c05b80fdd7e01ee.
  • PR head: 6fd5bbd, tree f56b0d35a2f2ab997f7c0b89f7f851a0818cf454.

Checks described here used Go 1.25.1 on Linux amd64 with isolated fixtures and networking disabled during behavior tests. Existing repository tests remain unchanged; the extra reproductions below are deliberately outside the committed source. These are local execution results, not physical-device validation. The PR's Checks tab provides the published workflow result.

Linux tests cover binary/empty persistence, real subprocess contention, stable inode, new/existing permissions, GC-disabled descriptor counts, lock-open/rename failures, and temporary Write/Sync plus directory Sync errors. Normal/lifecycle race checks, vet and repository tests pass. The portable check below passes all three modeled close-error cases. Additional cleanup injection confirms earlier ENOSPC survives a cleanup Close EIO, with no temporary pathname/owned-descriptor leak. No throughput claim follows from cleanup changes.

Use Linux Go 1.25.1, Python 3 and cached dependencies from the PR head listed above. Save this complete script outside the checkout as reproduce-close.py, then run python3 /path/to/reproduce-close.py from the checkout. It creates the Go overlay, fixtures and generated caches in its own temporary directory, preserves the selected cached module input, and exercises the real unmodified params source.

#!/usr/bin/env python3
# Run from an exact PR checkout: python3 /path/to/reproduce-close.py
# Requires Linux, Go 1.25.1, Python 3, and the checkout's cached Go modules.
# All generated files and params live in a new temporary directory.
import json
import os
import pathlib
import subprocess
import tempfile

repository = pathlib.Path.cwd().resolve()
assert (repository / 'params/params.go').is_file()
environment = dict(os.environ, GOPROXY='off', GOSUMDB='off', GOTOOLCHAIN='local')
go_environment = json.loads(subprocess.check_output(['go', 'env', '-json', 'GOROOT', 'GOMODCACHE'], env=environment, text=True))
goroot = pathlib.Path(go_environment['GOROOT'])
environment['GOMODCACHE'] = go_environment['GOMODCACHE']
stdlib = goroot / 'src/os/file_posix.go'
source = stdlib.read_text()
needle = '\treturn f.file.close()'
assert source.count(needle) == 1
injected = '''	err := f.file.close()
	name := f.Name()
	prefix, exact := Getenv("PARAM_CLOSE_PREFIX"), Getenv("PARAM_CLOSE_PATH")
	if err == nil && ((prefix != "" && len(name) >= len(prefix) && name[:len(prefix)] == prefix) || (exact != "" && name == exact)) {
		return &PathError{Op: "close", Path: name, Err: syscall.EIO}
	}
	return err'''
test = r'''package params
import (
  "errors"
  "os"
  "path/filepath"
  "syscall"
  "testing"
)
func TestCloseErrorContract(t *testing.T) {
  for _, scenario := range []string{"temporary", "directory put", "directory remove"} {
    t.Run(scenario, func(t *testing.T) {
      directory := filepath.Join(t.TempDir(), "d")
      if err := os.Mkdir(directory, 0700); err != nil { t.Fatal(err) }
      path := filepath.Join(directory, "FixtureParam")
      if err := os.WriteFile(path, []byte("original"), 0600); err != nil { t.Fatal(err) }
      if scenario == "temporary" {
        t.Setenv("PARAM_CLOSE_PREFIX", filepath.Join(directory, ".tmp_value_"))
      } else { t.Setenv("PARAM_CLOSE_PATH", directory) }
      var err error
      if scenario == "directory remove" { err = RemoveParam(path) } else { err = PutParam(path, []byte("replacement")) }
      t.Logf("%s error=%v", scenario, err)
      if !errors.Is(err, syscall.EIO) { t.Errorf("Close EIO not returned: %v", err) }
      if scenario == "temporary" {
        data, readErr := os.ReadFile(path)
        t.Logf("published bytes=%q", data)
        if readErr != nil || string(data) != "original" { t.Errorf("failed-close data published: %q, %v", data, readErr) }
      }
    })
  }
}
'''
with tempfile.TemporaryDirectory(prefix='param-close-repro-') as directory:
  scratch = pathlib.Path(directory)
  (scratch / 'os_file_posix.go').write_text(source.replace(needle, injected))
  (scratch / 'close_test.go').write_text(test)
  overlay = {'Replace': {
    str(stdlib): str(scratch / 'os_file_posix.go'),
    str(repository / 'params/close_contract_test.go'): str(scratch / 'close_test.go'),
  }}
  (scratch / 'overlay.json').write_text(json.dumps(overlay))
  (scratch / 'tmp').mkdir()
  environment['TMPDIR'] = str(scratch / 'tmp')
  environment['GOCACHE'] = str(scratch / 'build-cache')
  environment['GOPATH'] = str(scratch / 'gopath')
  result = subprocess.run(['go', 'test', '-mod=readonly', '-count=1', '-v',
    '-overlay=' + str(scratch / 'overlay.json'), '-run=^TestCloseErrorContract$', './params'],
    env=environment, timeout=180)
  raise SystemExit(result.returncode)

Combined validation: exact source tree a4c306906627db3ac7a8ab768651c8628d55465a combines #101 9d61f06a1288ec4ea6f74f7d56a3057444316a13, #103 20e7c25b054b6399360676a7f539a39b4fbf855c, #105 bfcfe77be066634e36054327b20cfa6541063b54, #107 8e5e677d1196838069e9665d4e9d962bcc1e116b, #116 6fd5bbd6cf617c24a7fefd5e302fd36688a1a63b, #136 30e8ce98ea7a4c8401dbb5bfc62120c84fc689e4. The only overlapping file is settings/download.go; the resolution retains #136's selected-row loop and #107's progress publication inside it.

Combined Linux amd64 tests (including the scratch regression fixtures), race checks, vet and build passed. Under ARM64 emulation, the existing Makefile build stage (make GO_CAPNP_PATH=/usr/local/go-capnp/std), committed repository tests, vet and both CLI help commands passed with Go1.25.1; go.mod/go.sum stayed unchanged and the resulting executable is AArch64. The ARM64 run does not include the extra amd64 scratch tests. It used an isolated retained build image, not a new dependency-install/image rebuild or physical device. No archive payload or live params were accessed. #105 still requires runtime-first rollout before regenerated tiles are distributed.

@FrogAi
FrogAi force-pushed the codex/stabilize-params-locking branch from edaa76f to 4a8942f Compare August 10, 2026 02:58
@FrogAi
FrogAi force-pushed the codex/stabilize-params-locking branch from 4a8942f to 28621ff Compare September 4, 2026 21:28
FrogAi added a commit to FrogAi/mapd that referenced this pull request Sep 4, 2026
Retain the original PR commits and the tested rewrite. The resulting
file tree is identical to 28621ff.
Replace the earlier implementation with the simplified version.
@FrogAi
FrogAi force-pushed the codex/stabilize-params-locking branch from ea137ed to 6034c72 Compare September 4, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants