Skip to content

fix(puffin): guard ClearProperties after Finish#1373

Merged
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/puffin-clearproperties-finished-state
Jul 4, 2026
Merged

fix(puffin): guard ClearProperties after Finish#1373
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/puffin-clearproperties-finished-state

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Problem

Writer.ClearProperties in puffin could be called after Finish and silently mutate state because there was no finalization guard.

That is inconsistent with other mutation methods and weakens writer lifecycle safety.

Fix

  • Change (*Writer).ClearProperties signature to return error.
  • Add a finalization check:
    • when w.done == true, return errors.New("puffin: cannot clear properties: writer already finalized")
  • Keep normal pre-finish behavior unchanged (w.props is cleared and call succeeds).
  • Update tests to assert:
    • ClearProperties returns nil before finish
    • ClearProperties returns an error after finish
  • Note: this is a small API-surface change because callers now handle an error return.

Validation

  • go test ./puffin -run TestClearProperties -count=1
  • go test ./puffin -run "Test(SetCreatedBy|ClearProperties|AddPropertiesAfterFinish)" -count=1

Scope

No behavior change for active writer usage; only enforces lifecycle consistency after finalization.

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 4, 2026 14:57

@tanmayrauth tanmayrauth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. ClearProperties after Finish now returns an error and no-ops, matching the existing post-finalize guards. Good test coverage.

@zeroshade zeroshade merged commit 03ae002 into apache:main Jul 4, 2026
15 checks passed
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.

3 participants